-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
Feature/QueryGroup Add Create API in plugin #14459
base: main
Are you sure you want to change the base?
Feature/QueryGroup Add Create API in plugin #14459
Conversation
Signed-off-by: Kaushal Kumar <[email protected]>
Signed-off-by: Kaushal Kumar <[email protected]>
…odes Signed-off-by: Kaushal Kumar <[email protected]>
Signed-off-by: Kaushal Kumar <[email protected]>
Signed-off-by: Ruirui Zhang <[email protected]>
Signed-off-by: Ruirui Zhang <[email protected]>
Signed-off-by: Ruirui Zhang <[email protected]>
Signed-off-by: Ruirui Zhang <[email protected]>
Signed-off-by: Ruirui Zhang <[email protected]>
Signed-off-by: Ruirui Zhang <[email protected]>
Signed-off-by: Ruirui Zhang <[email protected]>
Signed-off-by: Ruirui Zhang <[email protected]>
Signed-off-by: Ruirui Zhang <[email protected]>
Signed-off-by: Ruirui Zhang <[email protected]>
Signed-off-by: Ruirui Zhang <[email protected]>
Signed-off-by: Ruirui Zhang <[email protected]>
Signed-off-by: Ruirui Zhang <[email protected]>
Signed-off-by: Ruirui Zhang <[email protected]>
a2bd381
to
cb836d7
Compare
❌ 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? |
❌ 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? |
A couple high level comments:
|
Thanks for the info! I'll look into it |
- [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)) |
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.
Merge issue?
* compatible open source license. | ||
*/ | ||
|
||
package org.opensearch.plugin.wlm; |
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 package be plugin.wlm.action
? That would be consistent with the structure in core, I guess.
/** | ||
* Name for CreateQueryGroupAction | ||
*/ | ||
public static final String NAME = "cluster:admin/opensearch/query_group/wlm/_create"; |
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 have wlm
prior to query_group
in the action path?
/** | ||
* Rest action to create QueryGroup | ||
* |
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.
I don't think this logic is for Rest Action. Probably Transport layer action
* @param queryGroup - The QueryGroup to be created | ||
* @param restStatus - The resStatus for the response |
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 QueryGroup created. Since, by the time of response, it is already created, so to be is incorrect
nit: typo in resStatus
builder.startObject(); | ||
builder.field("name", queryGroup.getName()); | ||
builder.field("mode", queryGroup.getMode().getName()); | ||
builder.field("updatedAt", queryGroup.getUpdatedAtInMillis()); | ||
builder.mapContents(queryGroup.getResourceLimits()); | ||
builder.endObject(); |
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 logic should be delegated to QueryGroup class
private final QueryGroup queryGroup; | ||
private RestStatus restStatus; |
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.
Is there any other class example that follows this data object, restStatus response object model?
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:
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.