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

Allow coordinator run auto compaction duty period to be configured separately from other indexing duties #12263

Merged
merged 13 commits into from
Feb 19, 2022
Merged

Allow coordinator run auto compaction duty period to be configured separately from other indexing duties #12263

merged 13 commits into from
Feb 19, 2022

Conversation

maytasm
Copy link
Contributor

@maytasm maytasm commented Feb 15, 2022

Allow coordinator run auto compaction duty period to be configured separately from other indexing duties

Description

User may want to run auto compaction duty more frequently. Auto compaction does already have task slot limits (max slot and ratio) to prevent auto compaction from starving other ingestion tasks. Hence, it is safe to run auto compaction more frequently so that we can consistently utilized all the task slots available to auto compaction. However, before this change auto compaction duty is tied to other indexing duties and changing the period for auto compaction will also impact the other duties. This PR uses #11601 functionality that allows coordinator to be dynamically enabled via the configs. With this PR change, user can run auto compaction duty in it's own coordinator group using the following config:

druid.coordinator.dutyGroups=["compaction"]
druid.coordinator.compaction.duties=["compactSegments"]
druid.coordinator.compaction.period=PT60S

This PR has:

  • been self-reviewed.
  • added documentation for new or modified features or behaviors.
  • added Javadocs for most classes and all non-trivial methods. Linked related entities via Javadoc links.
  • added or updated version, license, or notice information in licenses.yaml
  • 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.

jihoonson
jihoonson reviewed Feb 17, 2022
View reviewed changes
Copy link
Contributor

@jihoonson jihoonson left a comment

Choose a reason for hiding this comment

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

LGTM overall, just left some comments about log message and test. It would also be nice to add some docs for this change.

server/src/main/java/org/apache/druid/server/coordinator/DruidCoordinator.java Outdated
return new CompactSegments(config, objectMapper, indexingServiceClient);
} else {
if (compactSegmentsDutyFromCustomGroups.size() > 1) {
log.warn("More than one compactSegments duty is configured in the Coordinator Custom Duty Group");
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
log.warn("More than one compactSegments duty is configured in the Coordinator Custom Duty Group");
log.warn("More than one compactSegments duty is configured in the Coordinator Custom Duty Group. The first duty will be picked up.");

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

server/src/main/java/org/apache/druid/server/coordinator/duty/CompactSegments.java
@@ -90,10 +92,11 @@
private final AtomicReference<Map<String, AutoCompactionSnapshot>> autoCompactionSnapshotPerDataSource = new AtomicReference<>();

@Inject
@JsonCreator
Copy link
Contributor

Choose a reason for hiding this comment

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

Please add a serde test for this class.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

jihoonson
jihoonson approved these changes Feb 17, 2022
View reviewed changes
Copy link
Contributor

@jihoonson jihoonson left a comment

Choose a reason for hiding this comment

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

LGTM. +1 after CI passes. Please check the CI failure.

docs/design/coordinator.md Outdated
@@ -98,6 +98,13 @@ Compaction tasks might fail due to the following reasons.

Once a compaction task fails, the Coordinator simply checks the segments in the interval of the failed task again, and issues another compaction task in the next run.

Note that Compacting Segments Coordinator Duty is automatically enabled and run as part of the Indexing Service Duties group. However, Compacting Segments Coordinator Duty can be configured to run in isolation as a seperate coordinator duty group. This allow changing the period of Compacting Segments Coordinator Duty without impacting the period of other Indexing Service Duties. This can be done by setting the following properties (for more details see [custom pluggable Coordinator Duty](../development/modules.md#Adding-your-own-custom-pluggable-Coordinator-Duty)):
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Note that Compacting Segments Coordinator Duty is automatically enabled and run as part of the Indexing Service Duties group. However, Compacting Segments Coordinator Duty can be configured to run in isolation as a seperate coordinator duty group. This allow changing the period of Compacting Segments Coordinator Duty without impacting the period of other Indexing Service Duties. This can be done by setting the following properties (for more details see [custom pluggable Coordinator Duty](../development/modules.md#Adding-your-own-custom-pluggable-Coordinator-Duty)):
Note that Compacting Segments Coordinator Duty is automatically enabled and run as part of the Indexing Service Duties group. However, Compacting Segments Coordinator Duty can be configured to run in isolation as a seperate coordinator duty group. This allows changing the period of Compacting Segments Coordinator Duty without impacting the period of other Indexing Service Duties. This can be done by setting the following properties (for more details see [custom pluggable Coordinator Duty](../development/modules.md#Adding-your-own-custom-pluggable-Coordinator-Duty)):

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@maytasm maytasm merged commit 6e2eded into apache:master Feb 19, 2022
@maytasm maytasm deleted the IMPLY-16466 branch February 19, 2022 07:03
@abhishekagarwal87 abhishekagarwal87 added this to the 0.23.0 milestone May 11, 2022
@abhishekagarwal87 abhishekagarwal87 mentioned this pull request May 25, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jihoonson jihoonson jihoonson approved these changes

Assignees
No one assigned
Labels
Area - Compaction
Projects
None yet
Milestone
0.23.0
Development

Successfully merging this pull request may close these issues.
3 participants
@maytasm @jihoonson @abhishekagarwal87