Skip to content

Make CompactionSearchPolicy injectable#13842

Merged
suneet-s merged 2 commits intoapache:masterfrom
suneet-s:continuous-compaction
Feb 27, 2023
Merged

Make CompactionSearchPolicy injectable#13842
suneet-s merged 2 commits intoapache:masterfrom
suneet-s:continuous-compaction

Conversation

@suneet-s
Copy link
Copy Markdown
Contributor

Description

A small refactoring that makes the search policy for compaction injectable.

Future changes can introduce new search policies that can be configured and injected so that operators can choose which search policy is best suited for their cluster.

This will also allow us to de-couple the scheduling of compaction jobs from the CompactSegments duty, allowing the co-ordinator to schedule compaction jobs faster than the duty lifecycle.

This PR is made so that it easy to review the future changes.


This PR has:

  • been self-reviewed.
  • 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.

A small refactoring that makes the search policy for compaction injectable.

Future changes can introduce new search policies that can be configured and
injected so that operators can choose which search policy is best suited for
their cluster.

This will also allow us to de-couple the scheduling of compaction jobs from
the CompactSegments duty, allowing the co-ordinator to schedule compaction
jobs faster than the duty lifecycle.

This PR is made so that it easy to review the future changes.
@kfaraz
Copy link
Copy Markdown
Contributor

kfaraz commented Feb 24, 2023

It makes sense to have the policy be injectable since we might have more search policies down the line.

This will also allow us to de-couple the scheduling of compaction jobs from the CompactSegments duty, allowing the co-ordinator to schedule compaction jobs faster than the duty lifecycle.

Could you please elaborate this part? I assume the intention here is not to make CompactSegments have a different period than the coordinator period.indexingPeriod as that is already possible through custom duties.

@paul-rogers
Copy link
Copy Markdown
Contributor

It appears that the policy is injected, which means the same policy for the entire cluster. Is there a need to allow the policy to be selected per task to better meed the needs per-datasource? We'd register the set of policies, and each task would name one of them, with some default.

@suneet-s
Copy link
Copy Markdown
Contributor Author

Could you please elaborate this part? I assume the intention here is not to make CompactSegments have a different period than the coordinator period.indexingPeriod as that is already possible through custom duties.

If CompactSegments runs faster than the segment metadata refresh interval (which I think is 1 min by default) it doesn't realize the segments that were selected for compaction were compacted already, and the task fails until the metadata is refreshed. Now that the policy is available in the coordinator, the CompactSegments duty can be split into 2 - one that refreshes the iterator which can take a long time and another that keeps polling for the next available interval to be compacted and schedules the compaction task if there is capacity on the cluster to do so. I'll try to write up something more detailed in the next PR.

@suneet-s
Copy link
Copy Markdown
Contributor Author

It appears that the policy is injected, which means the same policy for the entire cluster. Is there a need to allow the policy to be selected per task to better meed the needs per-datasource? We'd register the set of policies, and each task would name one of them, with some default.

This is something I had thought about working towards. Today, there is a cluster wide configuration for max number of compaction task slots. So the policy is used as a tie breaker across datasources.

In the future, it could make sense for a datasource to be able to say it prefers a search policy like "most fragmented intervals first" vs another datasource that cares about "newest interval first". But the cluster will need to decide how to choose which policy to apply when comparing intervals across these datasources. This sent my head spinning, so I figured I'd try to cross that bridge when I came to it.

Copy link
Copy Markdown
Contributor

@kfaraz kfaraz left a comment

Choose a reason for hiding this comment

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

LGTM 🚀

@suneet-s
Copy link
Copy Markdown
Contributor Author

Overruling travis, the license failure was fixed in #13845

@suneet-s suneet-s merged commit 31c7de1 into apache:master Feb 27, 2023
@suneet-s suneet-s deleted the continuous-compaction branch February 27, 2023 15:57
gianm added a commit to gianm/druid that referenced this pull request Feb 27, 2023
Crept in during apache#13842. Possibly logical conflict with another PR.
@gianm gianm mentioned this pull request Feb 27, 2023
gianm added a commit that referenced this pull request Feb 27, 2023
Crept in during #13842. Possibly logical conflict with another PR.
@clintropolis clintropolis added this to the 26.0 milestone Apr 10, 2023
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.

5 participants