-
Notifications
You must be signed in to change notification settings - Fork 3.7k
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
Fix NPE in CompactSegments #16713
Fix NPE in CompactSegments #16713
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the fix @kfaraz
@@ -234,12 +234,13 @@ private boolean cancelTaskIfGranularityChanged( | |||
Granularity configuredSegmentGranularity = dataSourceCompactionConfig.getGranularitySpec() | |||
.getSegmentGranularity(); | |||
Granularity taskSegmentGranularity = compactionTaskQuery.getGranularitySpec().getSegmentGranularity(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The fix makes sense. It seems that the members and getters in UserCompactionTaskGranularityConfig
are missing @Nullable
annotations. Could we also update those?
Also, for this bug fix, it'll be good to update an existing test in CompactSegmentsTest
or another relevant test to pass null values to the compaction task query.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some of the @Nullable
annotations are being added in another PR #16291 , so I skipped them here.
Okay if we add the tests separately? I want to merge this PR and not wait for another CI build.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Works for me 👍
Thanks for the prompts reviews, @AmatyaAvadhanula , @abhishekrb19 , @asdf2014 ! |
The NPE causes
CompactSegments
duty to fail and thus not submit any compaction tasks.