HotToWarmTieringService changes to tier shards by neetikasinghal · Pull Request #14891 · opensearch-project/OpenSearch

neetikasinghal · 2024-07-23T05:20:00Z

Description

Previous Draft PR: HotToWarmTieringService changes and changes in shard balancer to tier shards neetikasinghal/OpenSearch#144
Adds in the service side of the changes needed for hot to warm tiering.
Also tested the tiering changes end-to-end by pulling in recovery changes: Writable warm relocation recovery #14670

Related Issues

Check List

Functionality includes testing.
API changes companion pull request created, if applicable.
Public documentation issue/PR created, if applicable.

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.

github-actions · 2024-07-23T05:32:07Z

❌ Gradle check result for 2e9f80d: 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?

github-actions · 2024-07-23T05:53:55Z

❌ Gradle check result for 052d551: 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?

github-actions · 2024-07-23T22:18:42Z

❌ Gradle check result for 0813dac: 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?

server/src/main/java/org/opensearch/cluster/routing/RoutingPool.java

.../src/main/java/org/opensearch/cluster/routing/allocation/allocator/RemoteShardsBalancer.java

server/src/main/java/org/opensearch/node/Node.java

server/src/main/java/org/opensearch/indices/tiering/HotToWarmTieringService.java

github-actions · 2024-07-26T23:17:24Z

✅ Gradle check result for 8975e97: SUCCESS

github-actions · 2024-07-27T00:08:50Z

✅ Gradle check result for d7bffe7: SUCCESS

github-actions · 2024-07-27T00:14:27Z

❌ Gradle check result for 4774394: 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?

jed326

Needs a changelog entry but otherwise LGTM

...c/main/java/org/opensearch/action/admin/indices/tiering/TransportHotToWarmTieringAction.java

server/src/main/java/org/opensearch/indices/tiering/HotToWarmTieringService.java

server/src/main/java/org/opensearch/indices/tiering/TieringRequestValidator.java

server/src/main/java/org/opensearch/indices/tiering/HotToWarmTieringService.java

server/src/main/java/org/opensearch/action/admin/indices/tiering/TieringRequestContext.java

sohami · 2024-08-02T18:12:02Z

server/src/main/java/org/opensearch/action/admin/indices/tiering/TieringRequestContext.java

+@ExperimentalApi
+public class TieringRequestContext {
+    private final ActionListener<HotToWarmTieringResponse> actionListener;
+    private final Map<Index, IndexTieringInfo> indexTieringStatusMap;


how about just keeping 2 set for accepted and completed indices and 1 map for failedIndices. That way you can keep the indices in respective data structures and don't have to do filtering every time for indices in specific state.

I thought about the above as well. However, the current approach is cleaner in that the entries need not be moved from accepted set to completed/failed indices, the entry is only in one of the states of tiering (keeping only one source of truth).
Also, later if we plan to extend the tiering states, TieringRequestContext can be easily extensible for different states of tiering. In any case if there is another transition state introduced for another type of tiering, we would need to introduce another set whereas in current way, we just need to add a state to IndexTieringState.
I would like to keep it as is unless you have a strong opinion here.

However, the current approach is cleaner in that the entries need not be moved from accepted set to completed/failed indices, the entry is only in one of the states of tiering (keeping only one source of truth)

I don't see issue with moving from one set to another. Each data structure is providing an easy way to get the indices in that state which is what most of the calls from H2WTieringService is and hence the suggestion. We still have single source of truth which is TieringRequestContext object that encapsulates these different data structures to maintain indices in different states instead of a single map.

Also, later if we plan to extend the tiering states, TieringRequestContext can be easily extensible for different states of tiering. In any case if there is another transition state introduced for another type of tiering, we would need to introduce another set whereas in current way, we just need to add a state to IndexTieringState

Agree on this but don't see any other tiering state at the moment. Also TieringRequestContext is tied to HotToWarmMigration so if we have to reuse or introduce any new state for a different tiering type, then refactoring will be needed anyways. We can always think about the better mechanism when the use case with other tiering types are known.

However, the current approach is cleaner in that the entries need not be moved from accepted set to completed/failed indices, the entry is only in one of the states of tiering (keeping only one source of truth)

I don't see issue with moving from one set to another. Each data structure is providing an easy way to get the indices in that state which is what most of the calls from H2WTieringService is and hence the suggestion. We still have single source of truth which is TieringRequestContext object that encapsulates these different data structures to maintain indices in different states instead of a single map.

I agree that for a given request we have a single source of truth which is TieringRequestContext. However, to figure out the state of the index (accepted/completed/failed), we would have different sources of truth.
Given that we would have a limited number of indices that would undergo tiering at a given time, I see that the filtering operation would be a constant time operation. What is the other concern that you see with the current implementation?
Also with sets approach - we would need 3 sets and one map here - accepted, successful, completed, failed as compared to what is maintained as a single map in the current implementation.

Also, later if we plan to extend the tiering states, TieringRequestContext can be easily extensible for different states of tiering. In any case if there is another transition state introduced for another type of tiering, we would need to introduce another set whereas in current way, we just need to add a state to IndexTieringState

Agree on this but don't see any other tiering state at the moment. Also TieringRequestContext is tied to HotToWarmMigration so if we have to reuse or introduce any new state for a different tiering type, then refactoring will be needed anyways. We can always think about the better mechanism when the use case with other tiering types are known.

makes sense.

github-actions · 2024-08-07T01:37:54Z

✅ Gradle check result for 7da1aa6: SUCCESS

codecov · 2024-08-07T01:42:22Z

Codecov Report

❌ Patch coverage is 27.80488% with 148 lines in your changes missing coverage. Please review.
✅ Project coverage is 71.86%. Comparing base (97c1bf0) to head (d99f55f).
⚠️ Report is 1888 commits behind head on main.

Files with missing lines	Patch %	Lines
...earch/indices/tiering/HotToWarmTieringService.java	20.58%	106 Missing and 2 partials ⚠️
...n/admin/indices/tiering/TieringRequestContext.java	0.00%	24 Missing ⚠️
...dices/tiering/TransportHotToWarmTieringAction.java	46.15%	7 Missing ⚠️
...ices/tiering/TieringUpdateClusterStateRequest.java	0.00%	6 Missing ⚠️
...rch/action/admin/indices/tiering/TieringUtils.java	83.33%	1 Missing ⚠️
...org/opensearch/cluster/metadata/IndexMetadata.java	66.66%	1 Missing ⚠️
...earch/indices/tiering/TieringRequestValidator.java	0.00%	1 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##               main   #14891      +/-   ##
============================================
+ Coverage     71.74%   71.86%   +0.12%     
- Complexity    62904    62963      +59     
============================================
  Files          5178     5182       +4     
  Lines        295167   295359     +192     
  Branches      42679    42701      +22     
============================================
+ Hits         211774   212268     +494     
+ Misses        66011    65663     -348     
- Partials      17382    17428      +46

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

sohami

Can we also look into increasing the code coverage, seems pretty low right now. Lets aim to keep it above 80%.

server/src/main/java/org/opensearch/cluster/metadata/IndexNameExpressionResolver.java

server/src/main/java/org/opensearch/action/admin/indices/tiering/TieringUtils.java

sohami · 2024-08-07T06:12:09Z

...c/main/java/org/opensearch/action/admin/indices/tiering/TransportHotToWarmTieringAction.java

+        final TieringUpdateClusterStateRequest updateClusterStateRequest = new TieringUpdateClusterStateRequest(
+            tieringValidationResult.getRejectedIndices(),
+            request.waitForCompletion()
+        ).ackTimeout(request.timeout())


where is this ackTimeout used ?

refer: https://github.com/opensearch-project/OpenSearch/blob/main/server/src/main/java/org/opensearch/cluster/ack/ClusterStateUpdateRequest.java#L53

server/src/main/java/org/opensearch/indices/tiering/HotToWarmTieringService.java

Bukhtawar · 2024-08-07T08:41:45Z

server/src/main/java/org/opensearch/action/admin/indices/tiering/TieringRequestContext.java

+        }
+
+        public void markTiered() {
+            this.state = IndexTieringState.TIERED;
+        }


looks like a state machine, do we need to validate the transitions are happening in the correct sequence?

Signed-off-by: Neetika Singhal <neetiks@amazon.com>

github-actions · 2024-08-08T00:18:55Z

✅ Gradle check result for d99f55f: SUCCESS

neetikasinghal · 2024-08-09T02:26:30Z

server/src/main/java/org/opensearch/indices/tiering/HotToWarmTieringService.java

+    void processTieringRequestContexts(final ClusterState clusterState) {
+        final Map<Index, TieringRequestContext> tieredIndices = new HashMap<>();
+        for (TieringRequestContext tieringRequestContext : tieringRequestContexts) {
+            if (tieringRequestContext.isRequestProcessingComplete()) {


this check handles the cases where as indices are failed as part of the request

opensearch-trigger-bot · 2024-09-09T15:22:15Z

This PR is stalled because it has been open for 30 days with no activity.

opensearch-trigger-bot · 2024-10-13T15:22:06Z

This PR is stalled because it has been open for 30 days with no activity.

dbwiddis · 2024-10-14T00:18:30Z

@neetikasinghal Are you still working on this? Looks like we need a few merge conflicts addressed. Otherwise is it ready for review?

andrross · 2026-03-05T00:27:15Z

Closing due to age. @neetikasinghal please let me know if you intend to continue on this.

neetikasinghal requested review from Bukhtawar, CEHENKLE, Rishikesh1159, VachaShah, anasalkouz, andrross, ashking94, dblock, dbwiddis, gbbafna, kotwanikunal, mch2, msfroh, nknize, owaiskazi19, reta, sachinpkale, saratvemulapalli, shwetathareja and sohami as code owners July 23, 2024 05:20

neetikasinghal force-pushed the tiering-service branch from 2e9f80d to 052d551 Compare July 23, 2024 05:38

neetikasinghal force-pushed the tiering-service branch from 052d551 to 0813dac Compare July 23, 2024 21:23

neetikasinghal added backport 2.x Backport to 2.x branch v2.16.0 Issues and PRs related to version 2.16.0 release labels Jul 23, 2024

jed326 added the backport 2.16 label Jul 23, 2024

jed326 reviewed Jul 23, 2024

View reviewed changes

neetikasinghal force-pushed the tiering-service branch 2 times, most recently from d7bffe7 to 4774394 Compare July 26, 2024 23:39

neetikasinghal mentioned this pull request Jul 29, 2024

Add canRemain method to TargetPoolAllocationDecider to move shards from local to remote pool for hot to warm tiering #15010

Merged

3 tasks

prudhvigodithi mentioned this pull request Jul 30, 2024

[RELEASE] Release version 2.16.0 opensearch-project/opensearch-build#4771

Closed

74 tasks

jed326 reviewed Jul 30, 2024

View reviewed changes

sohami reviewed Aug 2, 2024

View reviewed changes

neetikasinghal force-pushed the tiering-service branch from 4774394 to 7da1aa6 Compare August 7, 2024 00:41

sohami reviewed Aug 7, 2024

View reviewed changes

Bukhtawar reviewed Aug 7, 2024

View reviewed changes

HotToWarmTieringService changes to tier shards

d99f55f

Signed-off-by: Neetika Singhal <neetiks@amazon.com>

neetikasinghal force-pushed the tiering-service branch from 7da1aa6 to d99f55f Compare August 7, 2024 23:27

neetikasinghal commented Aug 9, 2024

View reviewed changes

neetikasinghal mentioned this pull request Aug 15, 2024

HotToWarmTieringService changes to tier shards including queuing mechanism #15254

Draft

3 tasks

This was referenced Sep 6, 2024

[AUTOCUT] Gradle Check Flaky Test Report for MasterServiceTests #15809

Closed

[AUTOCUT] Gradle Check Flaky Test Report for WritableWarmIT #15846

Open

opensearch-trigger-bot bot added stalled Issues that have stalled and removed stalled Issues that have stalled labels Sep 9, 2024

opensearch-trigger-bot bot added the stalled Issues that have stalled label Oct 13, 2024

opensearch-trigger-bot bot removed the stalled Issues that have stalled label Oct 20, 2024

andrross closed this Mar 5, 2026

Conversation

neetikasinghal commented Jul 23, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Related Issues

Check List

Uh oh!

github-actions bot commented Jul 23, 2024

Uh oh!

github-actions bot commented Jul 23, 2024

Uh oh!

github-actions bot commented Jul 23, 2024

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

github-actions bot commented Jul 26, 2024

Uh oh!

github-actions bot commented Jul 27, 2024

Uh oh!

github-actions bot commented Jul 27, 2024

Uh oh!

jed326 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

sohami Aug 2, 2024

Choose a reason for hiding this comment

Uh oh!

neetikasinghal Aug 7, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sohami Aug 7, 2024

Choose a reason for hiding this comment

Uh oh!

neetikasinghal Aug 7, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Aug 7, 2024

Uh oh!

codecov bot commented Aug 7, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

sohami left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

sohami Aug 7, 2024

Choose a reason for hiding this comment

Uh oh!

neetikasinghal Aug 7, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Bukhtawar Aug 7, 2024

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Aug 8, 2024

Uh oh!

neetikasinghal Aug 9, 2024

Choose a reason for hiding this comment

Uh oh!

opensearch-trigger-bot bot commented Sep 9, 2024

Uh oh!

opensearch-trigger-bot bot commented Oct 13, 2024

Uh oh!

neetikasinghal commented Jul 23, 2024 •

edited

Loading

neetikasinghal Aug 7, 2024 •

edited

Loading

neetikasinghal Aug 7, 2024 •

edited

Loading

codecov bot commented Aug 7, 2024 •

edited

Loading