ES-10264: Bring over merge scheduler features from stateless #128155
Conversation
Add test for shouldSkipMerge Unused test code Add javadoc
elasticsearchmachine
added
the
needs:triage
BrianRothermich
added
>non-issue
:Distributed Indexing/Distributed
|
Pinging @elastic/es-distributed-indexing (Team:Distributed Indexing) |
elasticsearchmachine
added
the
Team:Distributed Indexing
elasticsearchmachine
added
the
serverless-linked
henningandersen
left a comment
henningandersen
left a comment
There was a problem hiding this comment.
I added comments for a less intrusive change by instead extending the merge scheduler in stateless, overriding a few specific methods.
| ThreadPoolMergeExecutorService threadPoolMergeExecutorService, | ||
| MergeMemoryEstimateProvider mergeMemoryEstimateProvider | ||
| MergeMemoryEstimateProvider mergeMemoryEstimateProvider, | ||
| BooleanSupplier shouldSkipMerge |
There was a problem hiding this comment.
I wonder if we should add a method shouldSkipMerge returning false here that we then implement in a stateless specific subclass instead?
| } | ||
|
|
||
| MergeSchedulerConfig(boolean autoThrottle, int maxThreadCount, int maxMergeCount) { | ||
| this.autoThrottle = autoThrottle; |
There was a problem hiding this comment.
Rather than overriding it like this, could we instead add ThreadPoolMergeScheduler.isAutoThrottle() that returns config.isAutoThrottle() by default but is overloaded in stateless to return false?
Similar for the two other fields.
| generationThresholdSize = scopedSettings.get(INDEX_TRANSLOG_GENERATION_THRESHOLD_SIZE_SETTING); | ||
| flushAfterMergeThresholdSize = scopedSettings.get(INDEX_FLUSH_AFTER_MERGE_THRESHOLD_SIZE_SETTING); | ||
| mergeSchedulerConfig = new MergeSchedulerConfig(this); | ||
| if (DiscoveryNode.isStateless(nodeSettings)) { |
There was a problem hiding this comment.
I prefer to avoid this if we can and added suggestions to use a sub-class for the merge scheduler in stateless.
This feels like a cleaner approach - thanks for the suggestion. Updated to use overridable methods |
BrianRothermich
removed
the
serverless-linked
Relates to an effort to consolidate the stateless merge scheduler with the current (stateful) merge scheduler from main ES. This PR brings over features required to maintain parity with the stateless scheduler. Specifically, a few methods are added for the stateless scheduler to override: Adds an overridable method shouldSkipMerge to test for skipping merges Adds 2 additional lifecycle callbacks to the scheduler for when a merge is enqueued and when a merge is executed or aborted. This is used by stateless to track active + queued merges per-shard Adds overridable methods for enabling/disabling IO/thread/merge count throttling Other functionality required by the stateless merge scheduler can use the existing callbacks from the stateful scheduler: beforeMerge can be overridden to prewarm afterMerge can be overridden to refresh after big merges Relates ES-10264 --------- Co-authored-by: elasticsearchmachine <[email protected]>
3 participants
Relates to an effort to consolidate the stateless merge scheduler with the current (stateful) merge scheduler from main ES. This PR brings over features required to maintain parity with the stateless scheduler. Specifically, a few methods are added for the stateless scheduler to override:
shouldSkipMergeto test for skipping mergesOther functionality required by the stateless merge scheduler can use the existing callbacks from the stateful scheduler:
beforeMergecan be overridden to prewarmafterMergecan be overridden to refresh after big mergesRelates ES-10264