-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
Delete API for weighted round robin search routing #4400
Delete API for weighted round robin search routing #4400
Conversation
Gradle Check (Jenkins) Run Completed with:
|
server/src/internalClusterTest/java/org/opensearch/search/SearchWRRIT.java
Outdated
Show resolved
Hide resolved
server/src/internalClusterTest/java/org/opensearch/search/SearchWRRIT.java
Outdated
Show resolved
Hide resolved
} | ||
|
||
} | ||
|
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.
should we remove the weights and then assert again that requests are equally distributed ?
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.
Sure, any pointers on which stat I can assert on for request distribution?
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.
sorry missed this. You can look for node/stats
API call for this.
* | ||
* @opensearch.internal | ||
*/ | ||
public class TransportGetWRRWeightsAction extends TransportClusterManagerNodeReadAction< |
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.
should we extend HandledTransportAction
instead of TransportClusterManagerNodeReadAction
? I do see that TransportClusterManagerNodeReadAction
are used for GET actions which needs cluster state . but just thinking about the benefits/cost for the same .
...org/opensearch/action/admin/cluster/shards/routing/wrr/get/TransportGetWRRWeightsAction.java
Outdated
Show resolved
Hide resolved
server/src/main/java/org/opensearch/client/ClusterAdminClient.java
Outdated
Show resolved
Hide resolved
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.
Delete API LGTM.
Gradle Check (Jenkins) Run Completed with:
|
Gradle Check (Jenkins) Run Completed with:
|
Gradle Check (Jenkins) Run Completed with:
|
Gradle Check (Jenkins) Run Completed with:
|
Gradle Check (Jenkins) Run Completed with:
|
Gradle Check (Jenkins) Run Completed with:
|
7b37962
to
7a8a4d2
Compare
Gradle Check (Jenkins) Run Completed with:
|
Gradle Check (Jenkins) Run Completed with:
|
Gradle Check (Jenkins) Run Completed with:
|
Gradle Check (Jenkins) Run Completed with:
|
Gradle Check (Jenkins) Run Completed with:
|
Codecov Report
@@ Coverage Diff @@
## main #4400 +/- ##
============================================
+ Coverage 70.66% 70.78% +0.12%
- Complexity 57578 57698 +120
============================================
Files 4661 4675 +14
Lines 276662 276918 +256
Branches 40325 40347 +22
============================================
+ Hits 195501 196016 +515
+ Misses 64926 64684 -242
+ Partials 16235 16218 -17
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
6554f3a
to
e864111
Compare
Gradle Check (Jenkins) Run Completed with:
|
Gradle Check (Jenkins) Run Completed with:
|
Signed-off-by: Anshu Agarwal <anshukag@amazon.com>
e864111
to
56fe6af
Compare
ActionListener.delegateFailure( | ||
listener, | ||
(delegatedListener, response) -> { | ||
delegatedListener.onResponse(new ClusterDeleteWeightedRoutingResponse(response.isAcknowledged())); |
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.
Why do we need to delegate this? The service method definition can be changed to ClusterDeleteWRResponse?
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.
Removed delegatedListener
logger.info("Deleting weighted routing metadata from the cluster state"); | ||
Metadata.Builder mdBuilder = Metadata.builder(currentState.metadata()); | ||
mdBuilder.removeCustom(WeightedRoutingMetadata.TYPE); | ||
return ClusterState.builder(currentState).metadata(mdBuilder).build(); |
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.
If we just wipe off the metadata will it internally handle to use ARS strategy? Or what will happen to subsequent requests?
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.
Yeah, subsequent requests will use ARS or attribute based routing. For multi-az clusters, currently ARS is not supported.
|
||
@Override | ||
public void onFailure(String source, Exception e) { | ||
logger.warn(() -> new ParameterizedMessage("failed to remove weighted routing metadata from cluster state [{}]", e)); |
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 msg looks like is exoecting a cluster state but provided an exception.
We can make this logger.error and pass the exception in it
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.
modified
Gradle Check (Jenkins) Run Completed with:
|
Signed-off-by: Anshu Agarwal <anshukag@amazon.com>
Gradle Check (Jenkins) Run Completed with:
|
Signed-off-by: Anshu Agarwal <anshukag@amazon.com>
Gradle Check (Jenkins) Run Completed with:
|
logger.error( | ||
() -> new ParameterizedMessage( | ||
"failed to remove weighted routing metadata from cluster state with an exception [{}]", | ||
e | ||
) | ||
); |
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.
if there's no other parameter we can keep it like - logger.error(msg, e)
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.
addressed
Gradle Check (Jenkins) Run Completed with:
|
5321b95
to
6a1daad
Compare
Gradle Check (Jenkins) Run Completed with:
|
Seeing unrelated test failures |
Signed-off-by: Anshu Agarwal <anshukag@amazon.com>
6a1daad
to
db8283c
Compare
Gradle Check (Jenkins) Run Completed with:
|
Gradle Check (Jenkins) Run Completed with:
|
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.
Thanks @anshu1106 for the changes
@Override | ||
public List<Route> routes() { | ||
return singletonList(new Route(DELETE, "/_cluster/routing/awareness/weights")); | ||
} |
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.
do we want to support explicit attribute deletion?
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.
yeah, but this will require few changes in the way data is persisted in cluster state as well. Create an issue to work on this #4747
Gradle Check (Jenkins) Run Completed with:
|
…t#4400) * Delete API for weighted round robin search routing Signed-off-by: Anshu Agarwal <anshukag@amazon.com>
…t#4400) * Delete API for weighted round robin search routing Signed-off-by: Anshu Agarwal <anshukag@amazon.com>
* Weighted round-robin scheduling policy for shard coordination traffic… (#4241) * Add PUT api to update shard routing weights (#4272) * Add GET api to get shard routing weights (#4275) * Fix weighted routing metadata deserialization error during node restart (#4691) * Delete API for weighted round robin search routing (#4400) * Mark apis experimental Signed-off-by: Anshu Agarwal <anshukag@amazon.com>
…t#4400) * Delete API for weighted round robin search routing Signed-off-by: Anshu Agarwal <anshukag@amazon.com>
Description
This pull request adds DELTE api to delete weights for weighted routing policy. This in turn disable the deature.
Issues Resolved
2859
3639
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.