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

Repair updated for edge case created with new directory structure. #78

Merged
merged 4 commits into from May 17, 2013

Conversation

matthewvon
Copy link
Contributor

@ghost ghost assigned matthewvon May 9, 2013

db_ptr=NULL;
options=org_options_;
options.block_cache=options_.block_cache;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there any danger in reusing this block cache for the level repair operation here? I haven't fully grasped what the code before this may put in it or where the repair may pull data out of it, but I was wondering if it was safer to just use a fresh one here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The block cache is global to the database (vnode). It's pointer has to be passed around different operations simply to give access. That is what you are seeing here. Good question though.

@engelsanchez
Copy link

After talking to @matthewvon about this we have agreed to avoid block cache reuse here to avoid any potential current or future weirdness if semantics change. In this case, it is only reused within the repair operation. When used from eleveldb, it is not sharing an existing block cache anyway (so not shared with the Riak vnode). So I'm giving this a 👍 💃 ⛵ conditional on the code changing to create a new block cache (options block_cache set to NULL). It will be taken away if any other naughtiness is introduced, however :)

@ghost ghost assigned engelsanchez May 17, 2013
Matthew V added 2 commits May 17, 2013 16:37
…, but there could be edge cases not seen or future code that would break assumptions.
matthewvon pushed a commit that referenced this pull request May 17, 2013
Repair updated for edge case created with new directory structure.
@matthewvon matthewvon merged commit 43ca5ad into master May 17, 2013
@matthewvon matthewvon deleted the mv-repair-by-level branch May 5, 2016 17:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants