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

Replica Count Validation when awareness replica balance is enabled #429

Merged
merged 3 commits into from
Aug 12, 2022

Conversation

gbbafna
Copy link
Contributor

@gbbafna gbbafna commented Jul 26, 2022

Signed-off-by: Gaurav Bafna gbbafna@amazon.com

Issue #, if available:

#381

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.

@gbbafna gbbafna requested a review from a team July 26, 2022 10:44
@gbbafna gbbafna changed the title Replica Count Validation when awareness replica balance is enabled [DRAFT] Replica Count Validation when awareness replica balance is enabled Jul 26, 2022
@gbbafna
Copy link
Contributor Author

gbbafna commented Jul 26, 2022

@bowenlan-amzn : This change depends on ongoing changes in OpenSearch . Is there a mechanism to run integ tests with local opensearch zip ?

I did try copying over the zip to ~/.gradle/caches/modules-2/files-2.1/org.opensearch.distribution.integ-test-zip/opensearch/2.1.0-SNAPSHOT/66102a4554a8e60c0c9ae5c82456ca04d850069e/opensearch-2.1.0-SNAPSHOT.zip , but still the compile is failing saying my changes are not present

@bowenlan-amzn
Copy link
Member

@bowenlan-amzn : This change depends on ongoing changes in OpenSearch . Is there a mechanism to run integ tests with local opensearch zip ?

I did try copying over the zip to ~/.gradle/caches/modules-2/files-2.1/org.opensearch.distribution.integ-test-zip/opensearch/2.1.0-SNAPSHOT/66102a4554a8e60c0c9ae5c82456ca04d850069e/opensearch-2.1.0-SNAPSHOT.zip , but still the integ test is failing over.

Didn't try to do such thing before. I think @saratvemulapalli may know the way to do this.

@saratvemulapalli
Copy link
Member

saratvemulapalli commented Jul 26, 2022

@gbbafna sure. Yeah you should be able to run via custom distribution.
Something like:

./gradlew build -PcustomDistributionUrl=https://ci.opensearch.org/ci/dbc/distribution-build-opensearch/2.1.1/latest/darwin/x64/tar/builds/opensearch/dist/opensearch-min-2.1.1-darwin-x64.tar.gz

Take a look at: https://github.com/opensearch-project/opensearch-plugins/blob/main/TESTING.md#distribution-download-plugin
Also take a look at: opensearch-project/OpenSearch#3637 (comment)

@gbbafna
Copy link
Contributor Author

gbbafna commented Jul 29, 2022

OpenSearch changes are merged. We can trigger rerun of the jobs once we have a snapshot build available .

@gbbafna
Copy link
Contributor Author

gbbafna commented Aug 2, 2022

@bowenlan-amzn : can you take a cursory look at the PR, while i try to fix the build issues ?

@saratvemulapalli : The opensearch changes are merged into main as well as 2.x branch. How do we get this changes to build with that snapshot ? I see that build files uses 2.1 branch snapshot . Do we need to backport it there as well ?

states = states
)
Assert.assertThrows(
ResponseException::class.java
Copy link
Member

Choose a reason for hiding this comment

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

nit: better if we can check the validation error message

@saratvemulapalli
Copy link
Member

saratvemulapalli commented Aug 3, 2022

@gbbafna Hmm what are you really looking for ?
If the changes are in 2.x, I would build this plugin with 2.2 and consume the latest snapshot of OpenSearch 2.2.

@gbbafna
Copy link
Contributor Author

gbbafna commented Aug 3, 2022

@gbbafna Hmm what are you really looking for ?
If the changes are in 2.x, I would build this plugin with 2.2 and consume the latest snapshot of OpenSearch 2.2.

2.2 branch in OpenSearch is not cut yet . It will be cut on 04/04 and there will be code freeze after that . So I am bit confused about the way forward here. I see 2.2 on OpenSearch now .

After the plugin changes to consume 2.2 snapshot is available , the gradle workflow should pass.

By the way, shouldn't the plugin use 2.1.1 in build.gradle instead of using 2.1.0 . That is the snapshot which OpenSearch regularly builds. Else, we would never able to build a feature in plugin, which depends on OpenSearch changes. After release 2.1.0 , OpenSearch immediately shifted to 2.1.1 , so even plugins should be doing the same. Thoughts ?

@saratvemulapalli
Copy link
Member

@gbbafna you are correct. As soon as the version is released there shouldn't be any development on that version.
The version has to be bumped. So yes, ISM should move to 2.1.1.

@gbbafna
Copy link
Contributor Author

gbbafna commented Aug 5, 2022

I tried to bump the version to 2.1.1, but it failed due to absence of notification plugin , similar to as observed in #443 .

I will wait for 2.2.0 PR #443 to go through and then re run gradle checks.

Signed-off-by: Ashish Agrawal <ashisagr@amazon.com>
Signed-off-by: Gaurav Bafna <gbbafna@amazon.com>
@codecov-commenter
Copy link

codecov-commenter commented Aug 8, 2022

Codecov Report

Merging #429 (ee31a25) into main (b8a77d4) will increase coverage by 0.18%.
The diff coverage is 78.94%.

@@             Coverage Diff              @@
##               main     #429      +/-   ##
============================================
+ Coverage     75.76%   75.94%   +0.18%     
- Complexity     2470     2474       +4     
============================================
  Files           314      314              
  Lines         14440    14454      +14     
  Branches       2233     2235       +2     
============================================
+ Hits          10940    10977      +37     
+ Misses         2255     2230      -25     
- Partials       1245     1247       +2     
Impacted Files Coverage Δ
...t/action/indexpolicy/TransportIndexPolicyAction.kt 80.32% <78.94%> (-0.23%) ⬇️
...nt/indexstatemanagement/model/destination/Slack.kt 55.55% <0.00%> (-22.23%) ⬇️
...gement/indexstatemanagement/action/ShrinkAction.kt 70.58% <0.00%> (-1.48%) ⬇️
.../rollup/action/start/TransportStartRollupAction.kt 71.76% <0.00%> (-1.18%) ⬇️
...nt/rollup/action/stop/TransportStopRollupAction.kt 76.47% <0.00%> (-1.18%) ⬇️
...nt/indexstatemanagement/ManagedIndexCoordinator.kt 69.29% <0.00%> (ø)
...ent/indexstatemanagement/util/ManagedIndexUtils.kt 77.87% <0.00%> (+1.76%) ⬆️
...management/indexstatemanagement/MetadataService.kt 64.92% <0.00%> (+2.23%) ⬆️
.../action/explain/TransportExplainTransformAction.kt 73.03% <0.00%> (+2.24%) ⬆️
...management/rollup/interceptor/RollupInterceptor.kt 79.36% <0.00%> (+3.17%) ⬆️
... and 1 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@gbbafna gbbafna changed the title [DRAFT] Replica Count Validation when awareness replica balance is enabled Replica Count Validation when awareness replica balance is enabled Aug 8, 2022
bowenlan-amzn
bowenlan-amzn previously approved these changes Aug 10, 2022
}
}
}

Copy link
Contributor

Choose a reason for hiding this comment

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

To keep coding style consistency, we can write this method as:

private fun validate() {
    request.policy.states.forEach { state ->
        state.actions.forEach { action ->
            if (action is ReplicaCountAction ) {
                val error = awarenessReplicaBalance.validate(action.numOfReplicas)
                if (error.isPresent) {
                    val ex = ValidationException()
                    ex.addValidationError(error.get())
                    actionListener.onFailure(ex)
                }
            }
        }
    }
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, done.

Signed-off-by: Gaurav Bafna <gbbafna@amazon.com>
@Angie-Zhang Angie-Zhang merged commit f64c0c7 into opensearch-project:main Aug 12, 2022
opensearch-trigger-bot bot pushed a commit that referenced this pull request Aug 12, 2022
)

* bump version to 2.2 (#446)

Signed-off-by: Ashish Agrawal <ashisagr@amazon.com>

* Replica Count validation when awareness replica balance is enabled

Signed-off-by: Gaurav Bafna <gbbafna@amazon.com>

* Addressing PR comments

Signed-off-by: Gaurav Bafna <gbbafna@amazon.com>

Signed-off-by: Ashish Agrawal <ashisagr@amazon.com>
Signed-off-by: Gaurav Bafna <gbbafna@amazon.com>
Co-authored-by: Ashish Agrawal <ashisagr@amazon.com>
(cherry picked from commit f64c0c7)
bowenlan-amzn pushed a commit that referenced this pull request Aug 16, 2022
) (#463)

* bump version to 2.2 (#446)

Signed-off-by: Ashish Agrawal <ashisagr@amazon.com>

* Replica Count validation when awareness replica balance is enabled

Signed-off-by: Gaurav Bafna <gbbafna@amazon.com>

* Addressing PR comments

Signed-off-by: Gaurav Bafna <gbbafna@amazon.com>

Signed-off-by: Ashish Agrawal <ashisagr@amazon.com>
Signed-off-by: Gaurav Bafna <gbbafna@amazon.com>
Co-authored-by: Ashish Agrawal <ashisagr@amazon.com>
(cherry picked from commit f64c0c7)

Co-authored-by: Gaurav Bafna <85113518+gbbafna@users.noreply.github.com>
@Angie-Zhang Angie-Zhang mentioned this pull request Sep 9, 2022
1 task
Angie-Zhang pushed a commit to Angie-Zhang/index-management that referenced this pull request Sep 12, 2022
…pensearch-project#429) (opensearch-project#463)

* bump version to 2.2 (opensearch-project#446)

Signed-off-by: Ashish Agrawal <ashisagr@amazon.com>

* Replica Count validation when awareness replica balance is enabled

Signed-off-by: Gaurav Bafna <gbbafna@amazon.com>

* Addressing PR comments

Signed-off-by: Gaurav Bafna <gbbafna@amazon.com>

Signed-off-by: Ashish Agrawal <ashisagr@amazon.com>
Signed-off-by: Gaurav Bafna <gbbafna@amazon.com>
Co-authored-by: Ashish Agrawal <ashisagr@amazon.com>
(cherry picked from commit f64c0c7)

Co-authored-by: Gaurav Bafna <85113518+gbbafna@users.noreply.github.com>
Signed-off-by: Angie Zhang <langelzh@amazon.com>
wuychn pushed a commit to ochprince/index-management that referenced this pull request Mar 16, 2023
…pensearch-project#429) (opensearch-project#463)

* bump version to 2.2 (opensearch-project#446)

Signed-off-by: Ashish Agrawal <ashisagr@amazon.com>

* Replica Count validation when awareness replica balance is enabled

Signed-off-by: Gaurav Bafna <gbbafna@amazon.com>

* Addressing PR comments

Signed-off-by: Gaurav Bafna <gbbafna@amazon.com>

Signed-off-by: Ashish Agrawal <ashisagr@amazon.com>
Signed-off-by: Gaurav Bafna <gbbafna@amazon.com>
Co-authored-by: Ashish Agrawal <ashisagr@amazon.com>
(cherry picked from commit f64c0c7)

Co-authored-by: Gaurav Bafna <85113518+gbbafna@users.noreply.github.com>
ronnaksaxena pushed a commit to ronnaksaxena/index-management that referenced this pull request Jul 19, 2023
…pensearch-project#429) (opensearch-project#463)

* bump version to 2.2 (opensearch-project#446)

Signed-off-by: Ashish Agrawal <ashisagr@amazon.com>

* Replica Count validation when awareness replica balance is enabled

Signed-off-by: Gaurav Bafna <gbbafna@amazon.com>

* Addressing PR comments

Signed-off-by: Gaurav Bafna <gbbafna@amazon.com>

Signed-off-by: Ashish Agrawal <ashisagr@amazon.com>
Signed-off-by: Gaurav Bafna <gbbafna@amazon.com>
Co-authored-by: Ashish Agrawal <ashisagr@amazon.com>
(cherry picked from commit f64c0c7)

Co-authored-by: Gaurav Bafna <85113518+gbbafna@users.noreply.github.com>
Signed-off-by: Ronnak Saxena <ronsax@amazon.com>
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.

6 participants