Feature/QueryGroup Add Create API in plugin by ruai0511 · Pull Request #14459 · opensearch-project/OpenSearch

ruai0511 · 2024-06-19T19:10:06Z

Description

This change introduces the Create QueryGroup API which we will use to enforce node level resiliency as part of this RFC: #12342.

The Create QueryGroup API schema is:

curl -XPUT "localhost:9200/_query_group/" -H 'Content-Type: application/json' -d ' 
{
     "name": "analytics",
     "jvm":0.4,
     "mode": "monitor"
}'

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.

Signed-off-by: Kaushal Kumar <ravi.kaushal97@gmail.com>

…odes Signed-off-by: Kaushal Kumar <ravi.kaushal97@gmail.com>

Signed-off-by: Kaushal Kumar <ravi.kaushal97@gmail.com>

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

github-actions · 2024-06-19T19:20:03Z

❌ Gradle check result for a2bd381: 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-06-19T20:02:20Z

❌ Gradle check result for 5b53934: 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-06-19T20:09:53Z

✅ Gradle check result for cb836d7: SUCCESS

codecov · 2024-06-19T20:10:51Z

Codecov Report

Attention: Patch coverage is 55.65032% with 208 lines in your changes missing coverage. Please review.

Project coverage is 71.63%. Comparing base (b15cb0c) to head (cb836d7).
Report is 454 commits behind head on main.

Files	Patch %	Lines
...opensearch/plugin/wlm/CreateQueryGroupRequest.java	57.65%	42 Missing and 5 partials ⚠️
...ugin/wlm/service/QueryGroupPersistenceService.java	51.28%	36 Missing and 2 partials ⚠️
.../search/query_group/QueryGroupServiceSettings.java	25.58%	31 Missing and 1 partial ⚠️
...pensearch/cluster/metadata/QueryGroupMetadata.java	42.22%	26 Missing ⚠️
...ch/plugin/wlm/rest/RestCreateQueryGroupAction.java	0.00%	15 Missing ⚠️
...va/org/opensearch/cluster/metadata/QueryGroup.java	86.84%	6 Missing and 9 partials ⚠️
...ch/plugin/wlm/TransportCreateQueryGroupAction.java	0.00%	12 Missing ⚠️
...pensearch/plugin/wlm/CreateQueryGroupResponse.java	65.00%	7 Missing ⚠️
...java/org/opensearch/cluster/metadata/Metadata.java	73.68%	5 Missing ⚠️
...pensearch/plugin/wlm/WorkloadManagementPlugin.java	0.00%	4 Missing ⚠️
... and 2 more

Additional details and impacted files

@@             Coverage Diff              @@
##               main   #14459      +/-   ##
============================================
+ Coverage     71.42%   71.63%   +0.21%     
- Complexity    59978    62190    +2212     
============================================
  Files          4985     5129     +144     
  Lines        282275   292298   +10023     
  Branches      40946    42234    +1288     
============================================
+ Hits         201603   209385    +7782     
- Misses        63999    65635    +1636     
- Partials      16673    17278     +605

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

andrross · 2024-06-19T20:32:20Z

A couple high level comments:

This PR is creating a new plugin in the core repo, contrary to the (recently introduced) convention. I don't see it mentioned in the RFC, but please explain (or link to an explanation) of why this plugin should be in the core repo as opposed to a separate repo.
Please look at [PROPOSAL] Highlight REST API Changes in PRs #14456 as this PR appears to be introducing substantial changes to the REST API. I would love to see these new APIs defined and documented in https://github.com/opensearch-project/opensearch-api-specification in a draft PR before reviewing the implementation.

ruai0511 · 2024-06-19T20:51:40Z

A couple high level comments:

This PR is creating a new plugin in the core repo, contrary to the (recently introduced) convention. I don't see it mentioned in the RFC, but please explain (or link to an explanation) of why this plugin should be in the core repo as opposed to a separate repo.

Please look at [PROPOSAL] Highlight REST API Changes in PRs #14456 as this PR appears to be introducing substantial changes to the REST API. I would love to see these new APIs defined and documented in https://github.com/opensearch-project/opensearch-api-specification in a draft PR before reviewing the implementation.

Thanks for the info! I'll look into it

kaushalmahi12 · 2024-06-20T14:27:36Z

Thanks @andrross for going through the PR and provide useful insights. I have updated the RFC for the reasoning behind moving this into a plugin. We had also discussed this with @msfroh.

jainankitk · 2024-06-24T20:07:46Z

CHANGELOG-3.0.md

+- [QueryGroup] Add QueryGroup CRUD APIs ([#13315](https://github.com/opensearch-project/OpenSearch/pull/13315))
+- [QueryGroup] Add QueryGroup Create API ([#14459](https://github.com/opensearch-project/OpenSearch/pull/14459))


Merge issue?

jainankitk · 2024-06-24T20:09:05Z

plugins/workload-management/src/main/java/org/opensearch/plugin/wlm/CreateQueryGroupAction.java

+ * compatible open source license.
+ */
+
+package org.opensearch.plugin.wlm;


Should package be plugin.wlm.action? That would be consistent with the structure in core, I guess.

jainankitk · 2024-06-24T20:50:32Z

plugins/workload-management/src/main/java/org/opensearch/plugin/wlm/CreateQueryGroupAction.java

+    /**
+     * Name for CreateQueryGroupAction
+     */
+    public static final String NAME = "cluster:admin/opensearch/query_group/wlm/_create";


Should we have wlm prior to query_group in the action path?

I think that is the right way since wlm is the umbrella under which these query groups come.

plugins/workload-management/src/main/java/org/opensearch/plugin/wlm/CreateQueryGroupAction.java

...ns/workload-management/src/main/java/org/opensearch/plugin/wlm/CreateQueryGroupResponse.java

jainankitk · 2024-06-24T21:14:24Z

...ns/workload-management/src/main/java/org/opensearch/plugin/wlm/CreateQueryGroupResponse.java

+    private final QueryGroup queryGroup;
+    private RestStatus restStatus;


Is there any other class example that follows this data object, restStatus response object model?

This one is NotSerializableExceptionWrapper. But I also see other classes that don't store reststatus and only store data.

plugins/workload-management/src/main/java/org/opensearch/plugin/wlm/CreateQueryGroupAction.java

kaushalmahi12 · 2024-07-02T22:44:23Z

plugins/workload-management/src/main/java/org/opensearch/plugin/wlm/CreateQueryGroupAction.java

+    /**
+     * Name for CreateQueryGroupAction
+     */
+    public static final String NAME = "cluster:admin/opensearch/query_group/wlm/_create";


I think that is the right way since wlm is the umbrella under which these query groups come.

...ins/workload-management/src/main/java/org/opensearch/plugin/wlm/CreateQueryGroupRequest.java

...ns/workload-management/src/main/java/org/opensearch/plugin/wlm/CreateQueryGroupResponse.java

kaushalmahi12 · 2024-07-08T18:30:09Z

plugins/workload-management/src/test/java/org/opensearch/plugin/wlm/QueryGroupTestUtils.java

+        for (String resourceName : resourceLimitMapOne.keySet()) {
+            assertTrue(resourceLimitMapTwo.containsKey(resourceName));
+            assertEquals(resourceLimitMapOne.get(resourceName), resourceLimitMapTwo.get(resourceName));


We can replace this with

Suggested change

for (String resourceName : resourceLimitMapOne.keySet()) {

assertTrue(resourceLimitMapTwo.containsKey(resourceName));

assertEquals(resourceLimitMapOne.get(resourceName), resourceLimitMapTwo.get(resourceName));

assertTrue(resourceLimitMapOne.keySet().containsAll(resourceLimitMapTwo.keySet()));

assertTrue(resourceLimitMapOne.values().containsAll(resourceLimitMapTwo.values()));

kaushalmahi12 · 2024-07-08T18:34:10Z

plugins/workload-management/src/test/java/org/opensearch/plugin/wlm/QueryGroupTestUtils.java

+        for (QueryGroup groupOne : listOne) {
+            String groupOneName = groupOne.getName();
+            List<QueryGroup> groupTwoList = listTwo.stream().filter(sb -> sb.getName().equals(groupOneName)).collect(Collectors.toList());
+            assertEquals(1, groupTwoList.size());
+            QueryGroup groupTwo = groupTwoList.get(0);
+            assertEquals(groupOne.getName(), groupTwo.getName());
+            assertEquals(groupOne.get_id(), groupTwo.get_id());
+            compareResourceLimits(groupOne.getResourceLimits(), groupTwo.getResourceLimits());
+            assertEquals(groupOne.getMode(), groupTwo.getMode());
+            assertEquals(groupOne.getUpdatedAtInMillis(), groupTwo.getUpdatedAtInMillis());
+        }


How about sorting these two lists and then doing the index wise comparison I think that would be more efficient ?
If the query group already implements equalsTo method then we should leverage that.

...management/src/main/java/org/opensearch/plugin/wlm/service/QueryGroupPersistenceService.java

ruai0511 · 2024-07-09T00:55:56Z

Move this to #14680 and close this PR

kaushalmahi12 and others added 17 commits April 11, 2024 21:15

created a skeleton plugin with functional Rest API

8217a77

Signed-off-by: Kaushal Kumar <ravi.kaushal97@gmail.com>

add sandbox service

019c5aa

Signed-off-by: Kaushal Kumar <ravi.kaushal97@gmail.com>

add resourceLimitGroupId propagation logic from coordinator to data n…

028b0ce

…odes Signed-off-by: Kaushal Kumar <ravi.kaushal97@gmail.com>

add sandbox schema

ace034c

Signed-off-by: Kaushal Kumar <ravi.kaushal97@gmail.com>

add resource limit group crud apis

b722313

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

unit tests for resource limit group

ea727b9

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

Merge branch 'main' into feature/resourceLimitGroup-CRUD-plugin

a41f10b

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

modify changelog

cd09285

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

add id, create and update timestamp to resource limit group

14e9d7b

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

Merge branch 'main' into feature/resourceLimitGroup-CRUD-plugin

df8035b

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

fix ClusterSettings format

5d57745

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

address PR comments

7050596

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

modify crud api based on comments

4182e07

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

modify CRUD API to fit the new schema and change name to QueryGroup

43dec36

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

Merge branch 'main' into feature/resourceLimitGroup-CRUD-plugin

fd9ecee

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

modify based on commit

d490c15

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

seperate PR to only include create API

5b53934

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

ruai0511 requested review from Bukhtawar, CEHENKLE, anasalkouz, andrross, dblock, dbwiddis, dreamer-89, gbbafna, kotwanikunal, mch2, msfroh, nknize and owaiskazi19 as code owners June 19, 2024 19:10

ruai0511 requested review from Rishikesh1159, VachaShah, reta, sachinpkale, saratvemulapalli, shwetathareja, sohami and tlfeng as code owners June 19, 2024 19:10

modify CHANGELOG-3.0.md

cb836d7

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

ruai0511 force-pushed the QueryGroup-CRUD-create branch from a2bd381 to cb836d7 Compare June 19, 2024 19:13

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

[AUTOCUT] Gradle Check Flaky Test Report for RemotePrimaryLocalRecoveryIT #14314

Open

jainankitk reviewed Jun 28, 2024

View reviewed changes

kaushalmahi12 reviewed Jul 8, 2024

View reviewed changes

ruai0511 closed this Jul 9, 2024

ruai0511 mentioned this pull request Jul 15, 2024

Add Create QueryGroup API Logic #14680

Merged

8 tasks

		- [QueryGroup] Add QueryGroup CRUD APIs ([#13315](https://github.com/opensearch-project/OpenSearch/pull/13315))
		- [QueryGroup] Add QueryGroup Create API ([#14459](https://github.com/opensearch-project/OpenSearch/pull/14459))

		private final QueryGroup queryGroup;
		private RestStatus restStatus;

Conversation

ruai0511 commented Jun 19, 2024

Description

Check List

Uh oh!

github-actions bot commented Jun 19, 2024

Uh oh!

github-actions bot commented Jun 19, 2024

Uh oh!

github-actions bot commented Jun 19, 2024

Uh oh!

codecov bot commented Jun 19, 2024

Codecov Report

Uh oh!

andrross commented Jun 19, 2024

Uh oh!

ruai0511 commented Jun 19, 2024

Uh oh!

kaushalmahi12 commented Jun 20, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ruai0511 commented Jul 9, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants