-
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
[Weighted Routing] Support for fail open stats #6041
[Weighted Routing] Support for fail open stats #6041
Conversation
Signed-off-by: Anshu Agarwal <anshukag@amazon.com>
Gradle Check (Jenkins) Run Completed with:
|
* | ||
* @opensearch.internal | ||
*/ | ||
public class FailOpenWeightedRoutingStats implements ToXContentFragment, Writeable { |
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.
Can we define a generic weighted shard robin stats
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.
Makes sense. Updated
Signed-off-by: Anshu Agarwal <anshukag@amazon.com>
Gradle Check (Jenkins) Run Completed with:
|
Gradle Check (Jenkins) Run Completed with:
|
@Nullable | ||
private WeightedRoutingStats weightedRoutingStats; |
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.
Since this is only related to Search, does it make sense to be placed in SearchStats
instead ?
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 makes sense. Tried to go through the flow but looks like since failopen stats is captured inside FetchPhase
, there isn't an easy way to capture this since the current implementation is just looking for fetchphase https://github.com/opensearch-project/OpenSearch/blob/main/server/src/main/java/org/opensearch/index/shard/SearchOperationListener.java. Will require some deeper understanding in this as we need to capture fail open stats in all the actions modified in https://github.com/opensearch-project/OpenSearch/blob/main/server/src/main/java/org/opensearch/index/shard/SearchOperationListener.java. Do you have any pointers on this?
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.
LGTM.
Signed-off-by: Anshu Agarwal <anshukag@amazon.com>
Gradle Check (Jenkins) Run Completed with:
|
Gradle Check (Jenkins) Run Completed with:
|
@@ -162,6 +166,11 @@ public NodeStats(StreamInput in) throws IOException { | |||
} else { | |||
clusterManagerThrottlingStats = null; | |||
} | |||
if (in.getVersion().onOrAfter(Version.CURRENT)) { |
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 should be 2_6
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.
mixed cluster tests fail with 2_6. https://build.ci.opensearch.org/job/gradle-check/10046/
@@ -356,6 +371,9 @@ public void writeTo(StreamOutput out) throws IOException { | |||
if (out.getVersion().onOrAfter(Version.V_2_6_0)) { | |||
out.writeOptionalWriteable(clusterManagerThrottlingStats); | |||
} | |||
if (out.getVersion().onOrAfter(Version.CURRENT)) { |
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.
Same 2_6
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.
mixed cluster tests fail with 2_6. https://build.ci.opensearch.org/job/gradle-check/10046/
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 needs a backport
@@ -46,6 +46,8 @@ public static FailAwareWeightedRouting getInstance() { | |||
return INSTANCE; | |||
} | |||
|
|||
public static WeightedRoutingStats weightedRoutingStats = new WeightedRoutingStats(); |
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.
public static member variables have caps on
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.
ack
// number of times fail open has to be executed for search requests | ||
private AtomicInteger failOpenCount; | ||
|
||
public WeightedRoutingStats() { |
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.
Can we create a singleton instance in this class that can be shared across consumers
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
Signed-off-by: Anshu Agarwal <anshukag@amazon.com>
Gradle Check (Jenkins) Run Completed with:
|
Codecov Report
📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more @@ Coverage Diff @@
## main #6041 +/- ##
============================================
- Coverage 70.77% 70.66% -0.12%
+ Complexity 58744 58643 -101
============================================
Files 4775 4776 +1
Lines 281000 281042 +42
Branches 40592 40597 +5
============================================
- Hits 198873 198586 -287
- Misses 65812 66080 +268
- Partials 16315 16376 +61
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
Gradle Check (Jenkins) Run Completed with:
|
Signed-off-by: Anshu Agarwal <anshukag@amazon.com>
b147164
to
45bcc6e
Compare
Gradle Check (Jenkins) Run Completed with:
|
Gradle Check (Jenkins) Run Completed with:
|
Signed-off-by: Anshu Agarwal <anshukag@amazon.com>
Gradle Check (Jenkins) Run Completed with:
|
|
public void resetFailOpenCount() { | ||
failOpenCount.set(0); | ||
} | ||
} |
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.
make package private
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.
won't compile since test
is in a different package.
* Support for fail open stats Signed-off-by: Anshu Agarwal <anshukag@amazon.com> (cherry picked from commit 26ec5fc) Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
* [Weighted Routing] Support for fail open stats (#6041) * Support for fail open stats Signed-off-by: Anshu Agarwal <anshukag@amazon.com> (cherry picked from commit 26ec5fc) Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> * Fix weighted routing stats version (#6128) Signed-off-by: Anshu Agarwal <anshukag@amazon.com> Co-authored-by: Anshu Agarwal <anshukag@amazon.com> --------- Signed-off-by: Anshu Agarwal <anshukag@amazon.com> Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> Co-authored-by: Anshu Agarwal <anshuagarwal11@gmail.com> Co-authored-by: Anshu Agarwal <anshukag@amazon.com>
Signed-off-by: Anshu Agarwal anshukag@amazon.com
Description
This PR adds support to capture fail open stats. Currently it captures number of times fail open is executed for search requests using weighted shard routing.
Issues Resolved
#5037
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.