[Workload Management] QueryGroup Stats API Logic #15777

ruai0511 · 2024-09-05T20:46:01Z

Description

This change introduces the QueryGroup Stats API as part of this RFC: #12342.

The QueryGroup Stats API schema is:

curl -XGET "localhost:9200/_wlm/stats"

Check List

New functionality includes testing.
- All tests pass
New functionality has been documented.
- New functionality has javadoc added
Failing checks are inspected and point to the corresponding known issue(s) (See: Troubleshooting Failing Builds)
Commits are signed per the DCO using --signoff
Commit changes are listed out in CHANGELOG.md file (See: Changelog)
Public documentation issue/PR created

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-09-05T21:35:15Z

❌ Gradle check result for f647487: 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-09-05T21:37:03Z

❌ Gradle check result for 88c9cff: 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-09-05T21:37:52Z

❌ Gradle check result for b66823c: 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-09-06T00:08:58Z

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

kaushalmahi12 · 2024-09-08T22:19:48Z

server/src/main/java/org/opensearch/action/admin/cluster/wlm/QueryGroupStatsResponse.java

+    }
+
+    @Override
+    protected List<QueryGroupStats> readNodesFrom(StreamInput in) throws IOException {


Will this send the node_id along with the QueryGroupStats

This returns List<QueryGroupStats> but we can use queryGroupStats.getNode().getId() to get node_id

kaushalmahi12 · 2024-09-08T22:24:03Z

...er/src/main/java/org/opensearch/action/admin/cluster/wlm/TransportQueryGroupStatsAction.java

+     * Inner QueryGroupStatsRequest
+     *
+     * @opensearch.experimental
+     */
+    public static class NodeQueryGroupStatsRequest extends TransportRequest {
+


Do we really need this as this wrapper is not doing anything additional and the underlying object type is already TransportRequest ?

I'm following the structure of TransportNodesStatsAction. NodeQueryGroupStatsRequest is used as a per-node action handler inside Transport Action, and QueryGroupStatsRequest represents the action for all nodes as a whole. The NodeQueryGroupStatsRequest serves as the container for the request sent to each node, allowing the framework to serialize and execute that request on the individual nodes.

kaushalmahi12 · 2024-09-08T22:28:53Z

server/src/main/java/org/opensearch/client/ClusterAdminClient.java

+     * QueryGroup stats of the cluster.
+     *
+     * @param request  The QueryGroupStatsRequest
+     * @param listener A listener to be notified with a result
+     */
+    void queryGroupStats(QueryGroupStatsRequest request, ActionListener<QueryGroupStatsResponse> listener);
+


Why should we add this method in the ClusterAdmin interface ? I think this API should be accessible to all so does it make sense to add to AbstractClient since NodeClient extends it.

the actual implementation is in AbstractClient . I see nodeStats is also following this pattern (abstract function in ClusterAdminClient.java and implementation in AbstractClient.java)

server/src/main/java/org/opensearch/rest/action/admin/cluster/RestQueryGroupStatsAction.java

kaushalmahi12 · 2024-09-08T22:36:12Z

server/src/main/java/org/opensearch/wlm/QueryGroupService.java

 */
 public class QueryGroupService {
    // This map does not need to be concurrent since we will process the cluster state change serially and update
    // this map with new additions and deletions of entries. QueryGroupState is thread safe
    private final Map<String, QueryGroupState> queryGroupStateMap;
+    private final TransportService transportService;


I think we should rely on just a Supplier<String> as we only need nodeId here

...r/src/test/java/org/opensearch/action/support/nodes/TransportQueryGroupStatsActionTests.java

github-actions · 2024-09-10T05:58:41Z

❌ Gradle check result for caec681: 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-09-10T06:32:27Z

❌ Gradle check result for 6144198: 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-09-10T21:06:25Z

❕ Gradle check result for a8bb3a5: UNSTABLE

Please review all flaky tests that succeeded after retry and create an issue if one does not already exist to track the flaky failure.

github-actions · 2024-09-10T21:11:12Z

❕ Gradle check result for 5240274: UNSTABLE

Please review all flaky tests that succeeded after retry and create an issue if one does not already exist to track the flaky failure.

codecov · 2024-09-10T21:12:25Z

Codecov Report

Attention: Patch coverage is 76.31579% with 27 lines in your changes missing coverage. Please review.

Project coverage is 71.82%. Comparing base (f8515c7) to head (41aadb5).
Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
...ction/admin/cluster/RestQueryGroupStatsAction.java	36.36%	7 Missing ⚠️
...tion/admin/cluster/wlm/QueryGroupStatsRequest.java	66.66%	6 Missing ⚠️
...in/cluster/wlm/TransportQueryGroupStatsAction.java	77.77%	4 Missing ⚠️
...ain/java/org/opensearch/wlm/QueryGroupService.java	78.94%	0 Missing and 4 partials ⚠️
...ion/admin/cluster/wlm/QueryGroupStatsResponse.java	90.00%	2 Missing ⚠️
.../org/opensearch/client/support/AbstractClient.java	0.00%	2 Missing ⚠️
...ch/wlm/WorkloadManagementTransportInterceptor.java	80.00%	0 Missing and 1 partial ⚠️
...java/org/opensearch/wlm/stats/QueryGroupStats.java	75.00%	1 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##               main   #15777      +/-   ##
============================================
- Coverage     71.89%   71.82%   -0.07%     
- Complexity    64211    64227      +16     
============================================
  Files          5272     5277       +5     
  Lines        300578   300670      +92     
  Branches      43432    43436       +4     
============================================
- Hits         216086   215959     -127     
- Misses        66707    66945     +238     
+ Partials      17785    17766      -19

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

sgup432 · 2024-09-10T22:10:32Z

@ruai0511 @kaushalmahi12
I guess the node stats URI should _wlm/query_group/stats? As it looks more appropriate and also aligns with what we discussed in the stats meta issue. And allows for further extension in case we want.

ruai0511 · 2024-09-10T22:23:38Z

@ruai0511 @kaushalmahi12 I guess the node stats URI should _wlm/query_group/stats? As it looks more appropriate and also aligns with what we discussed in the stats meta issue. And allows for further extension in case we want.

If we use GET _wlm/query_group/stats, it'll create confusion between getting the stats and getting a querygroup named stats as the path would be the same. So if we want to keep _wlm, we probably need to change it to _wlm/query_group_stats

github-actions · 2024-09-10T22:32:08Z

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

sgup432 · 2024-09-10T22:43:11Z

If we use GET _wlm/query_group/stats, it'll create confusion between getting the stats and getting a querygroup named stats as the path would be the same. So if we want to keep _wlm, we probably need to change it to _wlm/query_group_stats

I think it is upto user what kind of naming they want to use. In that case they can also use query_group_stats as a name which is weird and is also confusing in itself with current URI structure.

I think having _wlm/<metric>/<name>/stats allows for better URI structure and is more consistent with the existing APIs.
If someone wants to use stats as a name so be it, they can still fetch it via _wlm/query_group/stats/stats which doesn't break the API from logical point of view.

github-actions · 2024-09-10T23:31:57Z

✅ Gradle check result for 41aadb5: SUCCESS

Signed-off-by: Ruirui Zhang <mariazrr@amazon.com>

github-actions · 2024-09-17T00:34:39Z

❌ Gradle check result for 9714571: 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: Ruirui Zhang <mariazrr@amazon.com>

kaushalmahi12 · 2024-09-23T21:34:42Z

server/src/main/java/org/opensearch/wlm/QueryGroupService.java


-    public QueryGroupService() {
-        this(new HashMap<>());
+    @Inject


Is @Inject needed here since we are creating the instance in Node.java

github-actions · 2024-09-23T23:48:50Z

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

ruai0511 force-pushed the querygroupstats-PR branch 3 times, most recently from 88c9cff to b66823c Compare September 5, 2024 21:28

ruai0511 force-pushed the querygroupstats-PR branch from b66823c to 9c241b7 Compare September 5, 2024 21:36

ruai0511 added the backport 2.x Backport to 2.x branch label Sep 5, 2024

opensearch-ci-bot mentioned this pull request Sep 6, 2024

[AUTOCUT] Gradle Check Flaky Test Report for RemoteStoreMigrationSettingsUpdateIT #15810

Closed

kaushalmahi12 reviewed Sep 8, 2024

View reviewed changes

ruai0511 force-pushed the querygroupstats-PR branch from caec681 to 6144198 Compare September 10, 2024 05:29

ruai0511 force-pushed the querygroupstats-PR branch 2 times, most recently from a8bb3a5 to 5240274 Compare September 10, 2024 20:02

ruai0511 force-pushed the querygroupstats-PR branch from 9bbc037 to 41aadb5 Compare September 10, 2024 22:31

ruai0511 added 4 commits September 16, 2024 16:58

changelog

3eaadca

Signed-off-by: Ruirui Zhang <mariazrr@amazon.com>

address comments

38e38d6

Signed-off-by: Ruirui Zhang <mariazrr@amazon.com>

add tests

e856d18

Signed-off-by: Ruirui Zhang <mariazrr@amazon.com>

modify uri

9714571

Signed-off-by: Ruirui Zhang <mariazrr@amazon.com>

ruai0511 force-pushed the querygroupstats-PR branch from 41aadb5 to 9714571 Compare September 17, 2024 00:00

opensearch-ci-bot mentioned this pull request Sep 17, 2024

[AUTOCUT] Gradle Check Flaky Test Report for RemoteFsTimestampAwareTranslogTests #15818

Open

modify based on comments

2df37a3

Signed-off-by: Ruirui Zhang <mariazrr@amazon.com>

kaushalmahi12 approved these changes Sep 23, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Workload Management] QueryGroup Stats API Logic #15777

[Workload Management] QueryGroup Stats API Logic #15777

ruai0511 commented Sep 5, 2024 •

edited

Loading

github-actions bot commented Sep 5, 2024

github-actions bot commented Sep 5, 2024

github-actions bot commented Sep 5, 2024

github-actions bot commented Sep 6, 2024

kaushalmahi12 Sep 8, 2024

ruai0511 Sep 9, 2024 •

edited

Loading

kaushalmahi12 Sep 8, 2024

ruai0511 Sep 9, 2024

kaushalmahi12 Sep 8, 2024

ruai0511 Sep 9, 2024 •

edited

Loading

kaushalmahi12 Sep 8, 2024

github-actions bot commented Sep 10, 2024

github-actions bot commented Sep 10, 2024

github-actions bot commented Sep 10, 2024

github-actions bot commented Sep 10, 2024

codecov bot commented Sep 10, 2024 •

edited

Loading

sgup432 commented Sep 10, 2024

ruai0511 commented Sep 10, 2024 •

edited

Loading

github-actions bot commented Sep 10, 2024

sgup432 commented Sep 10, 2024

github-actions bot commented Sep 10, 2024

github-actions bot commented Sep 17, 2024

kaushalmahi12 Sep 23, 2024

github-actions bot commented Sep 23, 2024

[Workload Management] QueryGroup Stats API Logic #15777

Are you sure you want to change the base?

[Workload Management] QueryGroup Stats API Logic #15777

Conversation

ruai0511 commented Sep 5, 2024 • edited Loading

Description

Check List

github-actions bot commented Sep 5, 2024

github-actions bot commented Sep 5, 2024

github-actions bot commented Sep 5, 2024

github-actions bot commented Sep 6, 2024

kaushalmahi12 Sep 8, 2024

Choose a reason for hiding this comment

ruai0511 Sep 9, 2024 • edited Loading

Choose a reason for hiding this comment

kaushalmahi12 Sep 8, 2024

Choose a reason for hiding this comment

ruai0511 Sep 9, 2024

Choose a reason for hiding this comment

kaushalmahi12 Sep 8, 2024

Choose a reason for hiding this comment

ruai0511 Sep 9, 2024 • edited Loading

Choose a reason for hiding this comment

kaushalmahi12 Sep 8, 2024

Choose a reason for hiding this comment

github-actions bot commented Sep 10, 2024

github-actions bot commented Sep 10, 2024

github-actions bot commented Sep 10, 2024

github-actions bot commented Sep 10, 2024

codecov bot commented Sep 10, 2024 • edited Loading

Codecov Report

sgup432 commented Sep 10, 2024

ruai0511 commented Sep 10, 2024 • edited Loading

github-actions bot commented Sep 10, 2024

sgup432 commented Sep 10, 2024

github-actions bot commented Sep 10, 2024

github-actions bot commented Sep 17, 2024

kaushalmahi12 Sep 23, 2024

Choose a reason for hiding this comment

github-actions bot commented Sep 23, 2024

ruai0511 commented Sep 5, 2024 •

edited

Loading

ruai0511 Sep 9, 2024 •

edited

Loading

ruai0511 Sep 9, 2024 •

edited

Loading

codecov bot commented Sep 10, 2024 •

edited

Loading

ruai0511 commented Sep 10, 2024 •

edited

Loading