Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add code to clean up empty data and hintfiles #137

Merged
merged 7 commits into from
Feb 12, 2014
Merged

Conversation

evanmcc
Copy link
Contributor

@evanmcc evanmcc commented Feb 5, 2014

This fix is meant to fix #136. I am concerned about the potential for this to delete something in error, but I am not sure how best to test it/trigger it. I can't think of anything that would cause this to delete accessible data, but that doesn't mean there isn't a way.

There is another potential fix that doesn't involve deletion, but that will only work in 2.0 after #128 lands.

@slfritchie @engelsanchez if you could take a look, this is considered a blocker for 1.4.8.

@evanmcc
Copy link
Contributor Author

evanmcc commented Feb 5, 2014

A unit test has been requested, I'll work on that tomorrow.

If I have an EUnit test that fails like this, for example:

    *failed*
    in function bitcask:do_put/5 (src/bitcask.erl, line 1242)
    in call from bitcask:put/3 (src/bitcask.erl, line 244)
    in call from bitcask:'-gh137_regression_test/0-lc$^0/1-0-'/2 (src/bitcask.erl, line 2024)
    in call from bitcask:gh137_regression_test/0 (src/bitcask.erl, line 2024)
    **error:{badmatch,{error,{badmatch,{error,eexist}}}}

... also yields this error complaint that really isn't an error:

    =ERROR REPORT==== 5-Feb-2014::13:42:11 ===
    ** Generic server <0.258.0> terminating
    ** Last message in was {'DOWN',#Ref<0.0.0.6952>,process,<0.230.0>,normal}
    ** When Server state == {state,undefined,undefined}
    ** Reason for termination ==
    ** {{badmatch,{error,badarg}},
        [{bitcask_file,handle_info,2,[{file,"src/bitcask_file.erl"},{line,170}]},
         {gen_server,handle_msg,5,[{file,"gen_server.erl"},{line,604}]},
         {proc_lib,init_p_do_apply,3,[{file,"proc_lib.erl"},{line,239}]}]}

Don't use an "ok = file:close...." pattern match here.
@slfritchie
Copy link
Contributor

Evan, I've added:

  • An EUnit test
  • Fix a harmless-but-scary-looking shutdown complaint by bitcask_file's gen_server
  • Fix a lager call that was throwing an exception and causing an unexpected code path to be taken
  • Handle a hintfile parsing throw'n error that wasn't handled correctly
  • (not exactly related) Try to fix (again) the bitcask_qc_expiry model to avoid false positives

@evanmcc
Copy link
Contributor Author

evanmcc commented Feb 12, 2014

This looks good 👍

@evanmcc
Copy link
Contributor Author

evanmcc commented Feb 12, 2014

I share your worry about removal on opening. Would it make more sense to just move the file? Prepend bad- so that the file id info is retained?

@slfritchie
Copy link
Contributor

I've a patch that would delete files only if called by init_keydir(), when we know that we're not merging. But that patch is upset/full-of-merge-conflicts when the still-pending tombstone management branch review is finally finished. ... There are so many 1.6 branch commits that need to be reconciled with the ts mgmt branch, my head is about to explode. ... Awww heck, I'll add the patch here. If you like it, we'll keep it, otherwise I'll either apply an un-patch or reset the branch back to last +1'ed commit, 73242fc.

@evanmcc
Copy link
Contributor Author

evanmcc commented Feb 12, 2014

I think that's reasonable.

evanmcc added a commit that referenced this pull request Feb 12, 2014
add code to clean up empty data and hintfiles
@evanmcc evanmcc merged commit 230b6d6 into 1.6 Feb 12, 2014
@seancribbs seancribbs deleted the bugfix/eexist-issue branch April 1, 2015 22:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants