Fix for OOM in the Tombstone generating logic in MSQ #13893
Merged
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
paul-rogers
reviewed
Mar 7, 2023
...vice/src/main/java/org/apache/druid/indexing/common/task/batch/parallel/TombstoneHelper.java
Outdated
Show resolved
Hide resolved
| @@ -147,7 +147,7 @@ public void tombstoneIntervalsCreatedForReplaceWhenReplaceIsContainedInUsedInter | |||
| replaceGranularity | |||
| ); | |||
| Assert.assertEquals( | |||
| ImmutableSet.of(Intervals.of("2020-03-05/2020-03-06"), Intervals.of("2020-03-06/2020-03-07")), | |||
| ImmutableSet.of(Intervals.of("2020-03-05/2020-03-07")), | |||
There was a problem hiding this comment.
Are the tests complete? That is, do we have, say replace a day into ALL?
Replace a day into YEAR with:
- Day as Jan 1. (I.e. one tombstone on the right)
- Day as Feb. 1. (Two tombstones)
- Day as Dec. 31. (One tombstone on the left)
Do we have a case for the non-aligned case? Original is month, target is week. (Although, I know we don't want to support week. It is still a useful non-aligned test case.)
There was a problem hiding this comment.
Updated, the aligned test cases were there. Added a few test cases with the abovementioned criteria, and added the case which would have caused OOM earlier.
…n/task/batch/parallel/TombstoneHelper.java Co-authored-by: Paul Rogers <[email protected]>
10 tasks
317brian
pushed a commit
to 317brian/druid
that referenced
this pull request
Mar 10, 2023
fix OOMs using a different logic for generating tombstones --------- Co-authored-by: Paul Rogers <[email protected]>
asdf2014
added
the
Area - MSQ
10 tasks
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.
While generating tombstones for MSQ (#13706) the controller might run into OOM while trying to compute the intervals for which the tombstones need to be generated because we are materializing all the intervals which need to be marked by a tombstone, partitioned by granularity. So for a query that replaces over all, and the existing data source contains a segment for the eternity, this would generate a large number of intervals (given a finite granularity such as DAY).
This PR changes the behavior of the tombstones generating logic wherein for the overlap, we only generate completely disjointed tombstones so that the number of intervals for which tombstones need to be generated do not explode.
Release note
(none)
Key changed/added classes in this PR
TombstoneHelperTombstoneHelperTestThis PR has: