Prevent full compactions from conflicting with other compactions #7055
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| var inuse bool | ||
| for _, f := range files { | ||
| if _, ok := c.files[f]; ok { | ||
| inuse = true |
There was a problem hiding this comment.
Could you just return false from here instead of breaking and then checking and returning?
There was a problem hiding this comment.
You also wouldn't need the inuse variable either.
There was a problem hiding this comment.
I was going to say the same, but I wasn't sure if this was more idiomatic.
There was a problem hiding this comment.
Yeah. I'll rework. I had a different approach initially and this is leftover from that.
|
Looks good. Minor nit. Also, could there be tests added to verify this functionality? 👍 |
Using os.O_EXCL is safer than checking and then creating the file.
This causes full compactions to not run if the server is running, but after a restart they do run.
Normally, compactions do not conflict on the files they are compacting. If the full cold threshold is set very low, it can cause conflicts where two compactions compact the same files. The full compaction was the only place this could happen as it's planning is greedy. To make this safer for concurrent execution, the compaction tracks which files are current being compacted and prevents any new compactions from starting if the file set overlaps. Fixes #6595
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Required for all non-trivial PRs
Normally, compactions do not conflict on the files they are compacting.
If the full cold threshold is set very low, it can cause conflicts where
two compactions compact the same files. The full compaction was the
only place this could happen as it's planning is greedy and normally does
not run until the shard is cold for a long period of time.
To make this safer for concurrent execution, the compactor tracks which
files are currently being compacted and prevents any new compactions from
starting if the file set overlaps.
Fixes #6595