You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@druid.apache.org by "suneet-s (via GitHub)" <gi...@apache.org> on 2023/02/23 21:02:39 UTC
[GitHub] [druid] suneet-s opened a new pull request, #13842: Make CompactionSearchPolicy injectable
suneet-s opened a new pull request, #13842:
URL: https://github.com/apache/druid/pull/13842
### 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.
<hr>
This PR has:
- [x] been self-reviewed.
- [x] added comments explaining the "why" and the intent of the code wherever would not be obvious for an unfamiliar reader.
- [x] added unit tests or modified existing tests to cover new code paths, ensuring the threshold for [code coverage](https://github.com/apache/druid/blob/master/dev/code-review/code-coverage.md) is met.
- [x] added integration tests.
- [x] been tested in a test Druid cluster.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] suneet-s commented on pull request #13842: Make CompactionSearchPolicy injectable
Posted by "suneet-s (via GitHub)" <gi...@apache.org>.
suneet-s commented on PR #13842:
URL: https://github.com/apache/druid/pull/13842#issuecomment-1444769560
> 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] paul-rogers commented on pull request #13842: Make CompactionSearchPolicy injectable
Posted by "paul-rogers (via GitHub)" <gi...@apache.org>.
paul-rogers commented on PR #13842:
URL: https://github.com/apache/druid/pull/13842#issuecomment-1444741820
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 an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] kfaraz commented on pull request #13842: Make CompactionSearchPolicy injectable
Posted by "kfaraz (via GitHub)" <gi...@apache.org>.
kfaraz commented on PR #13842:
URL: https://github.com/apache/druid/pull/13842#issuecomment-1443247650
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] suneet-s commented on pull request #13842: Make CompactionSearchPolicy injectable
Posted by "suneet-s (via GitHub)" <gi...@apache.org>.
suneet-s commented on PR #13842:
URL: https://github.com/apache/druid/pull/13842#issuecomment-1446585656
Overruling travis, the license failure was fixed in #13845
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] suneet-s commented on pull request #13842: Make CompactionSearchPolicy injectable
Posted by "suneet-s (via GitHub)" <gi...@apache.org>.
suneet-s commented on PR #13842:
URL: https://github.com/apache/druid/pull/13842#issuecomment-1444759552
> 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] suneet-s merged pull request #13842: Make CompactionSearchPolicy injectable
Posted by "suneet-s (via GitHub)" <gi...@apache.org>.
suneet-s merged PR #13842:
URL: https://github.com/apache/druid/pull/13842
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org