Skip to content

Fix NPE in CompactSegments #16713

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.

Sign up for GitHub

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

Merged
asdf2014 merged 1 commit into from
Jul 10, 2024
Merged

Fix NPE in CompactSegments #16713

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 3 additions & 2 deletions server/src/main/java/org/apache/druid/server/coordinator/duty/CompactSegments.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -234,12 +234,13 @@ private boolean cancelTaskIfGranularityChanged(
Granularity configuredSegmentGranularity = dataSourceCompactionConfig.getGranularitySpec()
.getSegmentGranularity();
Granularity taskSegmentGranularity = compactionTaskQuery.getGranularitySpec().getSegmentGranularity();
Copy link
Copy Markdown
Contributor

@abhishekrb19 abhishekrb19 Jul 10, 2024 •
edited
Loading

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.

Copy link
Copy Markdown
Contributor Author

@kfaraz kfaraz Jul 10, 2024

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.

Copy link
Copy Markdown
Contributor

@abhishekrb19 abhishekrb19 Jul 10, 2024

Choose a reason for hiding this comment

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

Works for me 👍

if (configuredSegmentGranularity.equals(taskSegmentGranularity)) {
if (configuredSegmentGranularity == null
|| configuredSegmentGranularity.equals(taskSegmentGranularity)) {
return false;
}

LOG.info(
"Cancelling task [%s] as task segmentGranularity is [%s] but compaction config segmentGranularity is [%s]",
"Cancelling task[%s] as task segmentGranularity[%s] differs from compaction config segmentGranularity[%s].",
compactionTaskQuery.getId(), taskSegmentGranularity, configuredSegmentGranularity
);
overlordClient.cancelTask(compactionTaskQuery.getId());
Expand Down