-
Notifications
You must be signed in to change notification settings - Fork 2.3k
Adding configurable resiliency features to MergedSegmentWarmer #19629
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Adding configurable resiliency features to MergedSegmentWarmer #19629
Conversation
❌ Gradle check result for 3aeef75: FAILURE Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change? |
if (segmentSize < indexShard.getRecoverySettings().getMergedSegmentWarmerSegmentSizeThreshold().getBytes()) { | ||
return false; | ||
} | ||
|
||
return true; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this can be simplified to :
return segmentSize >= indexShard.getRecoverySettings().getMergedSegmentWarmerSegmentSizeThreshold().getBytes();
server/src/main/java/org/opensearch/indices/recovery/RecoverySettings.java
Outdated
Show resolved
Hide resolved
server/src/main/java/org/opensearch/indices/recovery/RecoverySettings.java
Outdated
Show resolved
Hide resolved
server/src/main/java/org/opensearch/index/engine/InternalEngine.java
Outdated
Show resolved
Hide resolved
if (segmentSize < indexShard.getRecoverySettings().getMergedSegmentWarmerSegmentSizeThreshold().getBytes()) { | ||
return false; | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lets add some trace/debug logs on the final return with diagnostic data
❌ Gradle check result for f0feabe: null Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change? |
f0feabe
to
34610d9
Compare
❌ Gradle check result for bbd9dd4: FAILURE Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change? |
Added a threshold property that controls which merged segments get pre-copied (warmed), ensuring only segments larger than the specified size are processed. Added cluster defaults for max_merge_count and max_merge_threads Signed-off-by: Aditya Khera <[email protected]>
bbd9dd4
to
2459597
Compare
❌ Gradle check result for 2459597: FAILURE Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change? |
Signed-off-by: Aditya Khera <[email protected]>
❌ Gradle check result for 36d0a37: FAILURE Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change? |
Signed-off-by: Aditya Khera <[email protected]>
❌ Gradle check result for 4aec3d9: FAILURE Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change? |
Signed-off-by: Aditya Khera <[email protected]>
❌ Gradle check result for b06e2ac: FAILURE Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change? |
Signed-off-by: Aditya Khera <[email protected]>
❌ Gradle check result for cd48be6: FAILURE Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change? |
Signed-off-by: Aditya Khera <[email protected]>
❌ Gradle check result for 2ce4ef8: FAILURE Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change? |
Signed-off-by: Aditya Khera <[email protected]>
❌ Gradle check result for 87cad2d: FAILURE Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change? |
❌ Gradle check result for f8cf13c: FAILURE Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change? |
Signed-off-by: Aditya Khera <[email protected]>
f8cf13c
to
2f943c3
Compare
❌ Gradle check result for 2f943c3: FAILURE Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change? |
public static final Setting<Integer> CLUSTER_DEFAULT_MAX_THREAD_COUNT_SETTING = new Setting<>( | ||
"cluster.default.index.merge.scheduler.max_thread_count", | ||
(s) -> Integer.toString(Math.max(1, Math.min(4, OpenSearchExecutors.allocatedProcessors(s) / 2))), | ||
(s) -> Setting.parseInt(s, 1, "cluster.default.index.merge.scheduler.max_thread_count"), | ||
Property.Dynamic, | ||
Property.NodeScope | ||
); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: Maybe good to remove default
in the settings name given it can be overridden, dynamically?
/** | ||
* Dynamic setting to set a threshold for minimum size of a merged segment to be warmed. | ||
*/ | ||
public static final Setting<ByteSizeValue> INDICES_REPLICATION_MERGES_WARMER_SEGMENT_SIZE_THRESHOLD_SETTING = Setting.byteSizeSetting( | ||
"indices.replication.merges.warmer.segment_size_threshold", | ||
new ByteSizeValue(500, ByteSizeUnit.MB), | ||
Property.Dynamic, | ||
Property.NodeScope | ||
); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: The name doesn't reflect a minimum
threshold size?
Description
[Describe what this change achieves]
Related Issues
Resolves #[Issue number to be closed when this PR is merged]
Check List
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.