Skip to content

Add new compaction config and status APIs served by the Overlord#17834

Merged
kfaraz merged 26 commits intoapache:masterfrom
kfaraz:fix_compaction_supervisors
Apr 2, 2025
Merged

Add new compaction config and status APIs served by the Overlord#17834
kfaraz merged 26 commits intoapache:masterfrom
kfaraz:fix_compaction_supervisors

Conversation

@kfaraz
Copy link
Copy Markdown
Contributor

@kfaraz kfaraz commented Mar 26, 2025 •
edited
Loading

Changes

  • Add new Overlord APIs to read and update compaction status and configs as follows:
    • If useSupervisors is true, read and write compaction supervisors
      using the CompactionScheduleror the SupervisorManager
    • If useSupervisors is false, read and write compaction configs using CoordinatorConfigManager
  • Move some logic from CoordinatorCompactionConfigsResource to CoordinatorConfigManager
  • Use CoordinatorConfigManager in OverlordCompactionResource to manipulate compaction configs
  • Add CompactionSupervisorManager to act as a wrapper over SupervisorManager and allow
    manipulation of compaction supervisor specs
  • Fix initialization bug in OverlordCompactionScheduler introduced in Use compaction dynamic config to enable compaction supervisors #17782

Release note

Add the following new experimental Overlord APIs:

Method Path
/druid/indexer/v1/compaction		 Description Required Permission
GET /config/cluster Get the cluster-level compaction config Read configs
POST /config/cluster Update the cluster-level compaction config Write configs
GET /config/datasources Get the compaction configs for all datasources Read datasource
GET /config/datasources/{dataSource} Get the compaction config of a single datasource Read datasource
POST /config/datasources/{dataSource} Update the compaction config of a single datasource Write datasource
GET /config/datasources/{dataSource}/history Get the compaction config history of a single datasource Read datasource
GET /status/datasources Get the compaction status of all datasources Read datasource
GET /status/datasources/{dataSource} Get the compaction status of a single datasource Read datasource

This PR has:

  • been self-reviewed.
  • added documentation for new or modified features or behaviors.
  • a release note entry in the PR description.
  • 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.

@kfaraz kfaraz requested a review from vogievetsky March 26, 2025 08:50
@kfaraz kfaraz added this to the 33.0.0 milestone Mar 26, 2025
@kfaraz kfaraz changed the title Add API to get cluster compaction config Add new compaction config and status APIs served by the Overlord Mar 30, 2025
github-advanced-security[bot]
github-advanced-security AI found potential problems Mar 30, 2025
View reviewed changes
Comment thread server/src/test/java/org/apache/druid/server/http/CoordinatorCompactionConfigsResourceTest.java
public void testSetCompactionTaskLimit()
{
final DruidCompactionConfig defaultConfig
resource.setCompactionTaskLimit(0.1, 100, mockHttpServletRequest);

Check notice

Code scanning / CodeQL

Deprecated method or constructor invocation Note test

Invoking
CoordinatorCompactionConfigsResource.setCompactionTaskLimit
Loading
should be avoided because it has been deprecated.
adarshsanjeev
adarshsanjeev reviewed Apr 1, 2025
View reviewed changes
Comment thread indexing-service/src/main/java/org/apache/druid/indexing/compact/CompactionScheduler.java
Comment thread server/src/main/java/org/apache/druid/server/compaction/BaseCandidateSearchPolicy.java
* a {@code priorityDatasource}.
*/
public abstract class BaseCandidateSearchPolicy
implements CompactionCandidateSearchPolicy, Comparator<CompactionCandidate>
Copy link
Copy Markdown
Contributor

@adarshsanjeev adarshsanjeev Apr 1, 2025

Choose a reason for hiding this comment

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

What's the reason to remove the comparator interface?

Copy link
Copy Markdown
Contributor Author

@kfaraz kfaraz Apr 1, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Keeping a plain Comparator made it difficult to find the places where the comparison was actually being used, since a search would show up all the usages of a Comparator.compare in the entire code base.
So I removed this interface and just added a new method compareCandidates instead of the usual compare method.

Comment thread ...e/src/main/java/org/apache/druid/indexing/overlord/http/CompactionConfigHistoryResponse.java
adarshsanjeev
adarshsanjeev approved these changes Apr 1, 2025
View reviewed changes
Copy link
Copy Markdown
Contributor

@adarshsanjeev adarshsanjeev left a comment

Choose a reason for hiding this comment

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

Overall, this seems fine. Thanks for adding tests for all the resources as it moving APIs is likely to break something.

Do the removed APIs also need to be mentioned in the PR description/patch notes?

@kfaraz
Copy link
Copy Markdown
Contributor Author

kfaraz commented Apr 1, 2025

Do the removed APIs also need to be mentioned in the PR description/patch notes?

@adarshsanjeev , I do plan to add documentation for new APIs today.
The only API that has been removed was experimental anyway. But if it is mentioned in the docs, I will update that too.

github-advanced-security[bot]
github-advanced-security AI found potential problems Apr 1, 2025
View reviewed changes
Comment thread ...ation-tests/src/main/java/org/apache/druid/testing/clients/CompactionResourceTestClient.java
Comment on lines +187 to +190
updateCompactionTaskSlot(
clusterConfig.getCompactionTaskSlotRatio(),
clusterConfig.getMaxCompactionTaskSlots() + 10
);

Check notice

Code scanning / CodeQL

Deprecated method or constructor invocation Note test

Invoking
CompactionResourceTestClient.updateCompactionTaskSlot
Loading
should be avoided because it has been deprecated.
Comment thread ...ation-tests/src/main/java/org/apache/druid/testing/clients/CompactionResourceTestClient.java
Comment on lines +191 to +194
updateCompactionTaskSlot(
clusterConfig.getCompactionTaskSlotRatio(),
clusterConfig.getMaxCompactionTaskSlots()
);

Check notice

Code scanning / CodeQL

Deprecated method or constructor invocation Note test

Invoking
CompactionResourceTestClient.updateCompactionTaskSlot
Loading
should be avoided because it has been deprecated.
@vogievetsky vogievetsky mentioned this pull request Apr 2, 2025
Merged
@kfaraz
Copy link
Copy Markdown
Contributor Author

kfaraz commented Apr 2, 2025

Merging as the CodeQL alerts are unrelated to the changes in this PR.

@kfaraz kfaraz merged commit 5d87655 into apache:master Apr 2, 2025
74 of 75 checks passed
@kfaraz kfaraz deleted the fix_compaction_supervisors branch April 2, 2025 16:41
@kfaraz
Copy link
Copy Markdown
Contributor Author

kfaraz commented Apr 2, 2025

Thanks a lot for sharing your thoughts on the APIs in this PR, @vogievetsky !
Thanks @adarshsanjeev for the review!

kfaraz pushed a commit that referenced this pull request Apr 3, 2025
@vogievetsky
Web console: support for supervisor based compaction (use new compact…
8511c55 
…ion APIs) (#17862)

This is a follow up to: #17834

This patch changes the console to use the new unified compaction APIs
and adds the setting to switch supervisor based compaction on and off.
@kfaraz kfaraz mentioned this pull request Apr 4, 2025
Merged
10 tasks
@soenkeliebau soenkeliebau mentioned this pull request Jul 24, 2025
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@adarshsanjeev adarshsanjeev adarshsanjeev approved these changes

@vogievetsky vogievetsky Awaiting requested review from vogievetsky

Assignees

No one assigned

Labels

Area - Documentation Area - Ingestion

Projects

None yet

Milestone

33.0.0

Development

Successfully merging this pull request may close these issues.

3 participants

@kfaraz @adarshsanjeev @github-advanced-security