Skip to content
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 CRUD APIs in plugin #13315

Conversation

ruai0511
Copy link
Contributor

@ruai0511 ruai0511 commented Apr 19, 2024

Description

As part of limiting the resource usage to the group of queries(QueryGroup). The feature is broken down into 3 independent parts.

  • CRUD APIs (Will go into the Plugin)
  • Request Tagging/Classification (Will go into the Plugin)
  • Tracking and Monitoring

This change consists of the CRUD APIs and the UTs

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"
}'

The Update QueryGroup API schema is:

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

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.

kaushalmahi12 and others added 6 commits April 11, 2024 21:15
Signed-off-by: Kaushal Kumar <ravi.kaushal97@gmail.com>
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>
Signed-off-by: Ruirui Zhang <mariazrr@amazon.com>
@ruai0511 ruai0511 changed the title Feature/resource limit group crud plugin Feature/QuerySandbox(ResourceLimitGroup) Add CRUD APIs in plugin Apr 19, 2024
Copy link
Contributor

❌ Gradle check result for ea727b9: 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>
Signed-off-by: Ruirui Zhang <mariazrr@amazon.com>
Signed-off-by: Ruirui Zhang <mariazrr@amazon.com>
Copy link
Contributor

❌ Gradle check result for 4182e07: 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>
@ruai0511 ruai0511 force-pushed the feature/resourceLimitGroup-CRUD-plugin branch from 6cc94e2 to 43dec36 Compare June 14, 2024 21:41
Signed-off-by: Ruirui Zhang <mariazrr@amazon.com>
Copy link
Contributor

❌ Gradle check result for 6cc94e2: 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?

Copy link
Contributor

❌ Gradle check result for 43dec36: 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?

Copy link
Contributor

❕ Gradle check result for fd9ecee: 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.

@jainankitk
Copy link
Collaborator

@ruai0511 - Thank you for creating this PR. Can you update the title/description with recent name (query group) and schema.

@jainankitk jainankitk closed this Jun 17, 2024
@jainankitk jainankitk reopened this Jun 17, 2024
Copy link
Collaborator

@jainankitk jainankitk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ruai0511 - This single PR has too many changes. May I suggest breaking this down into multiple PRs as below:

  • QueryGroup infra changes like PersistenceService, Pruner and other files
  • Create/Delete APIs for QueryGroup
  • Get/Update API changes for QueryGroup

In addition to making the code review much easier, keeping the PR short also makes it easy to root cause regressions (if any) discovered from the nightly benchmarks or any other mechanisms

CHANGELOG-3.0.md Show resolved Hide resolved
plugins/query-group/build.gradle Show resolved Hide resolved
plugins/query-group/build.gradle Show resolved Hide resolved
Comment on lines +31 to +38
/**
* A request for create QueryGroup
* User input schema:
* {
* "jvm": 0.4,
* "mode": "enforced",
* "name": "analytics"
* }
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Need to update the schema to most recent one

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This schema is the most recent one. The schema in QueryGroup (pasted below) is how we want to store the querygroup internally, it has id on the outside so users shouldn't use that schema when creating a querygroup.

/**
 * Class to define the QueryGroup schema
 * {
 *     "fafjafjkaf9ag8a9ga9g7ag0aagaga" : {
 *              "jvm": 0.4,
 *              "mode": "enforced",
 *              "name": "analytics",
 *              "updatedAt": 4513232415
 *      }
 * }
 */

Copy link
Contributor

❌ Gradle check result for fd9ecee: 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 ruai0511 changed the title Feature/QuerySandbox(ResourceLimitGroup) Add CRUD APIs in plugin Feature/QueryGroup Add CRUD APIs in plugin Jun 18, 2024
@ruai0511
Copy link
Contributor Author

@ruai0511 - This single PR has too many changes. May I suggest breaking this down into multiple PRs as below:

  • QueryGroup infra changes like PersistenceService, Pruner and other files
  • Create/Delete APIs for QueryGroup
  • Get/Update API changes for QueryGroup

In addition to making the code review much easier, keeping the PR short also makes it easy to root cause regressions (if any) discovered from the nightly benchmarks or any other mechanisms

I considered separating the

@ruai0511 - This single PR has too many changes. May I suggest breaking this down into multiple PRs as below:

  • QueryGroup infra changes like PersistenceService, Pruner and other files
  • Create/Delete APIs for QueryGroup
  • Get/Update API changes for QueryGroup

In addition to making the code review much easier, keeping the PR short also makes it easy to root cause regressions (if any) discovered from the nightly benchmarks or any other mechanisms

Turned out that I couldn't do a infra PR, because the persistence service depends on the API classes. I'm able to seperate this PR into four smaller PR though - each with one API.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants