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

Support for system index interface #789

Merged
merged 8 commits into from
May 30, 2023

Conversation

bowenlan-amzn
Copy link
Member

@bowenlan-amzn bowenlan-amzn commented May 24, 2023

Issue #, if available:

Description of changes:

CheckList:

  • Commits are signed per the DCO using --signoff

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.

@codecov
Copy link

codecov bot commented May 24, 2023

Codecov Report

Merging #789 (6dbf2ab) into 2.x (114910a) will decrease coverage by 0.35%.
The diff coverage is 28.57%.

@@             Coverage Diff              @@
##                2.x     #789      +/-   ##
============================================
- Coverage     76.09%   75.74%   -0.35%     
+ Complexity     2863     2846      -17     
============================================
  Files           364      364              
  Lines         16420    16426       +6     
  Branches       2364     2365       +1     
============================================
- Hits          12494    12442      -52     
- Misses         2553     2619      +66     
+ Partials       1373     1365       -8     
Impacted Files Coverage Δ
...exmanagement/opensearchapi/OpenSearchExtensions.kt 74.16% <0.00%> (-4.10%) ⬇️
...pensearch/indexmanagement/IndexManagementPlugin.kt 90.20% <100.00%> (+0.03%) ⬆️

... and 24 files with indirect coverage changes

Signed-off-by: bowenlan-amzn <[email protected]>
@bowenlan-amzn bowenlan-amzn marked this pull request as ready for review May 24, 2023 22:26
@@ -247,6 +247,8 @@ class RestStartRollupActionIT : RollupRestTestCase() {
assertEquals("Rollup is not RETRY", RollupMetadata.Status.RETRY, rollupMetadata.status)

// clearing the config index to prevent other tests using this multi shard index
Thread.sleep(2000L)
Copy link
Member

Choose a reason for hiding this comment

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

Why do we need to wait before deletion. Also adding sleeps in general tend to make tests flaky.

Copy link
Member Author

Choose a reason for hiding this comment

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

Before it's to wait for rollup metadata update to finish, observe the metadata update may recreate the index after deletion. But cannot guarantee it's the root cause.
This flaky is not related to the change in this PR.

Comment on lines +187 to +189
backoff = iter.next()
logger.warn("Rejected execution. Retrying in $backoff.", rje)
delay((backoff.millis))
Copy link
Member

Choose a reason for hiding this comment

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

Shall we add test to verify the behavior for this exception handling?

Copy link
Member Author

Choose a reason for hiding this comment

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

Don't feel there's need to check such simple logic 😛 no test for the existing block also

Copy link
Member

Choose a reason for hiding this comment

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

Its good to have unit tests for code coverage and setting the code expectation for future (or accidental) changes.

@@ -77,6 +77,11 @@ abstract class IndexManagementRestTestCase : ODFERestTestCase() {

protected fun Response.restStatus(): RestStatus = RestStatus.fromCode(this.statusLine.statusCode)

protected fun isIndexExists(index: String): Boolean {
Copy link
Member

Choose a reason for hiding this comment

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

Can we also assert on System Index property here to ensure it always remains a system index?

Copy link
Member Author

Choose a reason for hiding this comment

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

This is a REST API call.
It seems there's no way to check whether an index is system index using REST API.
https://github.com/opensearch-project/OpenSearch/blob/main/server/src/main/java/org/opensearch/indices/SystemIndices.java

@bowenlan-amzn bowenlan-amzn merged commit fa7e46f into opensearch-project:2.x May 30, 2023
opensearch-trigger-bot bot pushed a commit that referenced this pull request May 30, 2023
* Support for system index interface

Signed-off-by: bowenlan-amzn <[email protected]>

* Handle rejected execution exception as retryable in retry block

Signed-off-by: bowenlan-amzn <[email protected]>

* Fix tests

Signed-off-by: bowenlan-amzn <[email protected]>

* Fix tests

Signed-off-by: bowenlan-amzn <[email protected]>

---------

Signed-off-by: bowenlan-amzn <[email protected]>
(cherry picked from commit fa7e46f)
bowenlan-amzn added a commit that referenced this pull request May 31, 2023
* Support for system index interface

Signed-off-by: bowenlan-amzn <[email protected]>

* Handle rejected execution exception as retryable in retry block

Signed-off-by: bowenlan-amzn <[email protected]>

* Fix tests

Signed-off-by: bowenlan-amzn <[email protected]>

* Fix tests

Signed-off-by: bowenlan-amzn <[email protected]>

---------

Signed-off-by: bowenlan-amzn <[email protected]>
(cherry picked from commit fa7e46f)

Co-authored-by: bowenlan-amzn <[email protected]>
petardz pushed a commit to petardz/index-management that referenced this pull request May 31, 2023
…ch-project#799)

* Support for system index interface

Signed-off-by: bowenlan-amzn <[email protected]>

* Handle rejected execution exception as retryable in retry block

Signed-off-by: bowenlan-amzn <[email protected]>

* Fix tests

Signed-off-by: bowenlan-amzn <[email protected]>

* Fix tests

Signed-off-by: bowenlan-amzn <[email protected]>

---------

Signed-off-by: bowenlan-amzn <[email protected]>
(cherry picked from commit fa7e46f)

Co-authored-by: bowenlan-amzn <[email protected]>
ronnaksaxena pushed a commit to ronnaksaxena/index-management that referenced this pull request Jul 19, 2023
* Support for system index interface

Signed-off-by: bowenlan-amzn <[email protected]>

* Handle rejected execution exception as retryable in retry block

Signed-off-by: bowenlan-amzn <[email protected]>

* Fix tests

Signed-off-by: bowenlan-amzn <[email protected]>

* Fix tests

Signed-off-by: bowenlan-amzn <[email protected]>

---------

Signed-off-by: bowenlan-amzn <[email protected]>
Signed-off-by: Ronnak Saxena <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants