Add API to update cluster-level compaction configs #16803

kfaraz · 2024-07-25T11:01:38Z

Changes

Add API /druid/coordinator/v1/config/compaction/cluster to update cluster level compaction config
Add class CompactionConfigUpdateRequest
Fix bug in CoordinatorCompactionConfig which caused compaction engine to not be persisted.
Use json field name engine instead of compactionEngine because JSON field names must align
with the getter name.
Update MSQ validation error messages
Complete overhaul of CoordinatorCompactionConfigResourceTest to remove unnecessary mocking
and add more meaningful tests.
Add TuningConfigBuilder to easily build tuning configs for tests.
Add DatasourceCompactionConfigBuilder

Release notes

(Some of the details mentioned below are in a follow up PR #16810)

Add API to update cluster-level compaction dynamic config

Path: /druid/coordinator/v1/config/compaction/cluster
Method: POST
Sample Payload:

{
     "compactionTaskSlotRatio": 0.5,
     "maxCompactionTaskSlots": 10,
     "engine": "msq"
}

This API deprecates the older API /druid/coordinator/v1/config/compaction/taskslots.

This PR has:

server/src/test/java/org/apache/druid/server/coordinator/DataSourceCompactionConfigTest.java

+    final DataSourceCompactionConfig config = DataSourceCompactionConfig
+        .builder()
+        .forDataSource("dataSource")
+        .withInputSegmentSizeBytes(500L)
+        .withMaxRowsPerSegment(30)


server/src/test/java/org/apache/druid/server/coordinator/DataSourceCompactionConfigTest.java

+    final DataSourceCompactionConfig config = DataSourceCompactionConfig
+        .builder()
+        .forDataSource("dataSource")
+        .withInputSegmentSizeBytes(500L)
+        .withMaxRowsPerSegment(10000)


…ig_api

AmatyaAvadhanula · 2024-07-26T10:47:56Z

server/src/main/java/org/apache/druid/server/http/CoordinatorCompactionConfigsResource.java

+
+      final List<DataSourceCompactionConfig> datasourceConfigs = newConfig.getCompactionConfigs();
+      if (CollectionUtils.isNullOrEmpty(datasourceConfigs)
+          || current.getEngine() == newConfig.getEngine()) {


Is it possible for the payload to contain the current engine but incompatible configs?
For example the current engine is MSQ, and the update payload is also MSQ but with Native-only configs.

No, the update payload is only to update the cluster level configs. It does not contain any datasource level configs right now.

gargvishesh

Thanks for the PR @kfaraz. Overall changes LGTM. A few minor comments.

gargvishesh · 2024-07-26T11:58:13Z

.../main/java/org/apache/druid/server/coordinator/config/DataSourceCompactionConfigBuilder.java

+
+import java.util.Map;
+
+public class DataSourceCompactionConfigBuilder


Nit: Any particular reason to separate out the builder in a file of its own instead of adding to the DataSourceCompactionConfig file itself?

Not particularly, just didn't want to bloat up the DataSourceCompactionConfig class as it is currently a neat bean.
Since the builder is only 100 lines of code, I can put it in the same file too.
Let me know what you think.

I think changes to both would typically be in tandem, so would prefer to keep in the same file.

gargvishesh · 2024-07-26T12:29:48Z

server/src/main/java/org/apache/druid/server/http/CompactionConfigUpdateRequest.java

+  private final CompactionEngine compactionEngine;
+
+  @JsonCreator
+  public CompactionConfigUpdateRequest(


Would ClusterCompactionConfigUpdateRequest be a better name here?

It would be. I think we should also rename CoordinatorCompactionConfig to ClusterCompactionConfig or GlobalCompactionConfig. What do you think?

Also, would it be okay to do the rename changes and the API deprecation in a follow up PR?
(I have some more follow-up changes lined up).

Yes, particularly since it would no longer reside with the coordinator. Among the 2, I would prefer ClusterCompactionConfig

Cool, I guess we should rename the API to /cluster as well then.

gargvishesh · 2024-07-26T12:45:39Z

server/src/main/java/org/apache/druid/server/http/CoordinatorCompactionConfigsResource.java

-    UnaryOperator<CoordinatorCompactionConfig> operator =
-        current -> CoordinatorCompactionConfig.from(
-            current,
+    return updateClusterCompactionConfig(


Should we mark the above API as deprecated, now that there is also the /global endpoint?

Else engine should be supported here as well.

Hmm, fair point. Let me mark this API as deprecated.

gargvishesh

Approving as I'm okay with the discussed modifications being taken up in the follow-up PR as well.

gargvishesh · 2024-07-26T15:11:22Z

.../main/java/org/apache/druid/server/coordinator/config/DataSourceCompactionConfigBuilder.java

+
+import java.util.Map;
+
+public class DataSourceCompactionConfigBuilder


I think changes to both would typically be in tandem, so would prefer to keep in the same file.

gargvishesh · 2024-07-26T15:13:52Z

server/src/main/java/org/apache/druid/server/http/CompactionConfigUpdateRequest.java

+  private final CompactionEngine compactionEngine;
+
+  @JsonCreator
+  public CompactionConfigUpdateRequest(


Yes, particularly since it would no longer reside with the coordinator. Among the 2, I would prefer ClusterCompactionConfig

AmatyaAvadhanula

Thanks for the clarification, @kfaraz. LGTM

kfaraz · 2024-07-27T03:45:02Z

Thanks for the reviews, @gargvishesh , @AmatyaAvadhanula !

Changes: - Add API `/druid/coordinator/v1/config/compaction/global` to update cluster level compaction config - Add class `CompactionConfigUpdateRequest` - Fix bug in `CoordinatorCompactionConfig` which caused compaction engine to not be persisted. Use json field name `engine` instead of `compactionEngine` because JSON field names must align with the getter name. - Update MSQ validation error messages - Complete overhaul of `CoordinatorCompactionConfigResourceTest` to remove unnecessary mocking and add more meaningful tests. - Add `TuningConfigBuilder` to easily build tuning configs for tests. - Add `DatasourceCompactionConfigBuilder`

Add API to update compaction engine

4f7bf17

github-actions bot added the Area - Ingestion label Jul 25, 2024

Rename some methods

82a0b1f

github-advanced-security bot found potential problems Jul 25, 2024

View reviewed changes

kfaraz added 2 commits July 25, 2024 22:10

Merge branch 'master' of github.com:apache/druid into compaction_conf…

e961861

…ig_api

Fix tests, add utility methods, remove extra changes

c86bee9

github-actions bot added Area - Batch Ingestion Area - MSQ For multi stage queries - https://github.com/apache/druid/issues/12262 labels Jul 25, 2024

Remove NoopAuditManager

64f88a8

AmatyaAvadhanula reviewed Jul 26, 2024

View reviewed changes

gargvishesh reviewed Jul 26, 2024

View reviewed changes

gargvishesh approved these changes Jul 26, 2024

View reviewed changes

AmatyaAvadhanula approved these changes Jul 27, 2024

View reviewed changes

kfaraz merged commit caedeb6 into apache:master Jul 27, 2024
88 checks passed

kfaraz deleted the compaction_config_api branch July 27, 2024 03:45

kfaraz changed the title ~~Add API to update compaction engine~~ Add API to update cluster-level compaction configs Jul 29, 2024

kfaraz mentioned this pull request Aug 9, 2024

Run compaction as a supervisor on Overlord #16768

Merged

16 tasks

kfaraz added this to the 31.0.0 milestone Oct 4, 2024

kfaraz mentioned this pull request Oct 11, 2024

[DRAFT] 31.0.0 Release Notes #17332

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add API to update cluster-level compaction configs #16803

Add API to update cluster-level compaction configs #16803

kfaraz commented Jul 25, 2024 •

edited

Loading

AmatyaAvadhanula Jul 26, 2024

kfaraz Jul 26, 2024

gargvishesh left a comment

gargvishesh Jul 26, 2024

kfaraz Jul 26, 2024 •

edited

Loading

gargvishesh Jul 26, 2024

gargvishesh Jul 26, 2024

kfaraz Jul 26, 2024

gargvishesh Jul 26, 2024

kfaraz Jul 27, 2024

gargvishesh Jul 26, 2024

gargvishesh Jul 26, 2024

kfaraz Jul 26, 2024

gargvishesh left a comment

gargvishesh Jul 26, 2024

gargvishesh Jul 26, 2024

AmatyaAvadhanula left a comment

kfaraz commented Jul 27, 2024


		import java.util.Map;

		public class DataSourceCompactionConfigBuilder

Add API to update cluster-level compaction configs #16803

Add API to update cluster-level compaction configs #16803

Conversation

kfaraz commented Jul 25, 2024 • edited Loading

Changes

Release notes

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gargvishesh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kfaraz Jul 26, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gargvishesh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AmatyaAvadhanula left a comment

Choose a reason for hiding this comment

kfaraz commented Jul 27, 2024

kfaraz commented Jul 25, 2024 •

edited

Loading

kfaraz Jul 26, 2024 •

edited

Loading