Fix numDeletesToMerge for unchanged segments #13324
Conversation
| @@ -6128,7 +6128,7 @@ public final int numDeletesToMerge(SegmentCommitInfo info) throws IOException { | |||
| ensureOpen(false); | |||
| validate(info); | |||
| MergePolicy mergePolicy = config.getMergePolicy(); | |||
| final ReadersAndUpdates rld = getPooledInstance(info, false); | |||
| final ReadersAndUpdates rld = getPooledInstance(info, true); | |||
There was a problem hiding this comment.
If we pass create=true then we don't need to handle the case when rld is null below? There is a comment there saying that returning the number of hard deletes is safe, which confuses me since you're suggesting it's actually a problem? Maybe @s1monw can help shed a light?
There was a problem hiding this comment.
I just re-read the change that I made that added this comment and I think it's not allowed to pull a reader here if it's not already visible. I don't recall the side-effects of this but I am sure there are some. To me the test seems to be artificial since it's really just testing if that works on fresh writer?! I am not against the change I just think we need more coverage for this to ensure it's not triggering the same issues that caused the change in the first place?
I think IW should be very conservative here with pulling memory for segments that are not in memory yet.
Yet, the comment basically means that we can't return the soft del count since it might overcommit because of it's nature of being dynamic based on the MP.
|
This PR has not had activity in the past 2 weeks, labeling it as stale. If the PR is waiting for review, notify the [email protected] list. Thank you for your contribution! |
Currently, we do not trigger numDeletesToMerge for unchanged segments that have not yet entered the reader pool. Consequently, there are cases where force merges overlook segments with soft-deletes, as the merge policy only takes into account hard deletes when determining merge specifications.