Skip to content
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

Jump to bottom

Check targetCompactionSizeBytes to search for candidate segments in auto compaction #8495

Merged
merged 4 commits into from
Sep 10, 2019
Merged

Check targetCompactionSizeBytes to search for candidate segments in auto compaction #8495

merged 4 commits into from
Sep 10, 2019

Conversation

jihoonson
Copy link
Contributor

@jihoonson jihoonson commented Sep 10, 2019 •
edited
Loading

Description

This PR is to resolve the release blocker by temporarily addressing the bug reported in #8481. The auto compaction will check targetCompactionSizeBytes with this PR. The real solution should be something better such as #8489.

In the release notes, it should be mentioned that the auto compaction can get stuck if maxRowsPerSegment or maxTotalRows is set instead of targetCompactionSizeBytes.

This PR has:

  • been self-reviewed.
  • added Javadocs for most classes and all non-trivial methods. Linked related entities via Javadoc links.
  • added unit tests or modified existing tests to cover new code paths.
  • been tested in a test Druid cluster.

@jihoonson jihoonson added this to the 0.16.0 milestone Sep 10, 2019
clintropolis
clintropolis reviewed Sep 10, 2019
View reviewed changes
Copy link
Member

@clintropolis clintropolis left a comment

Choose a reason for hiding this comment

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

overall lgtm

server/src/main/java/org/apache/druid/server/coordinator/helper/SegmentCompactorUtil.java
return candidates
.stream()
.filter(segment -> segment.getSize() < minAllowedSize || segment.getSize() > maxAllowedSize)
.count() > 1;
Copy link
Member

@clintropolis clintropolis Sep 10, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

nit: I think this should use anyMatch instead of filter + count since the former might not have to materialize the entire set upon reaching first match

Copy link
Member

Choose a reason for hiding this comment

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

ignore ^

clintropolis
clintropolis approved these changes Sep 10, 2019
View reviewed changes
Copy link
Member

@clintropolis clintropolis left a comment

Choose a reason for hiding this comment

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

👍 after CI

@fjy fjy merged commit 762f4d0 into apache:master Sep 10, 2019
@clintropolis clintropolis mentioned this pull request Sep 10, 2019
Merged
clintropolis pushed a commit to clintropolis/druid that referenced this pull request Sep 10, 2019
@jihoonson @clintropolis
Check targetCompactionSizeBytes to search for candidate segments in a…
4b5d9e1 
…uto compaction (apache#8495)

* Check targetCompactionSizeBytes to search for candidate segments in auto compaction

* fix logs

* add javadoc

* rename
clintropolis added a commit that referenced this pull request Sep 10, 2019
@clintropolis
Check targetCompactionSizeBytes to search for candidate segments in a…
d901901 
…uto compaction (#8495) (#8500)

* Check targetCompactionSizeBytes to search for candidate segments in auto compaction

* fix logs

* add javadoc

* rename
This was referenced Sep 10, 2019
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@clintropolis clintropolis clintropolis approved these changes

@jon-wei jon-wei jon-wei approved these changes

Assignees
No one assigned
Labels
Bug Release Notes
Projects
None yet
Milestone
0.16.0
Development

Successfully merging this pull request may close these issues.
4 participants
@jihoonson @clintropolis @jon-wei @fjy