Skip to content

CompactionTask throws exception on conflicting segmentGranularity#10996

Merged
maytasm merged 2 commits intoapache:masterfrom
suneet-s:compact-granularity
Mar 16, 2021
Merged

CompactionTask throws exception on conflicting segmentGranularity#10996
maytasm merged 2 commits intoapache:masterfrom
suneet-s:compact-granularity

Conversation

@suneet-s
Copy link
Copy Markdown
Contributor

@suneet-s suneet-s commented Mar 16, 2021

Description

A follow up to #10843

Since there are 2 ways for a user to specify segmentGranularity - this change makes throws an exception if a user accidentally submits a compaction task with conflicting segmentGranularities - by using the old way and the new way at the same time.


This PR has:

  • been self-reviewed.
  • added documentation for new or modified features or behaviors.
  • added comments explaining the "why" and the intent of the code wherever would not be obvious for an unfamiliar reader.
  • added unit tests or modified existing tests to cover new code paths, ensuring the threshold for code coverage is met.
  • added integration tests.
  • been tested in a test Druid cluster.

@suneet-s suneet-s requested a review from maytasm March 16, 2021 01:25
this.segmentGranularity = segmentGranularity;
if (granularitySpec != null
&& segmentGranularity != null
&& !segmentGranularity.equals(granularitySpec.getSegmentGranularity())) {
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I couldn't decide what it means to have a granularitySpec.getSegmentGranularity() be null and segmentGranularity be non null, so I figured it would be good to flag that as a conflict as well

@suneet-s
Copy link
Copy Markdown
Contributor Author

  • No integration tests added since this is fully covered by the unit tests and it's a test of edge cases.
  • No documentation added because this would conflict with First refactor of compaction docs #10935 which is already in flight

@maytasm maytasm merged commit 6b0c2e8 into apache:master Mar 16, 2021
@suneet-s suneet-s deleted the compact-granularity branch March 16, 2021 20:31
@clintropolis clintropolis added this to the 0.22.0 milestone Aug 12, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants