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

Add allocation tests for searchable snapshot remote shards #5048

Conversation

kotwanikunal
Copy link
Member

Signed-off-by: Kunal Kotwani kkotwani@amazon.com

Description

  • Add allocation tests for searchable snapshot remote shards

Issues Resolved

Check List

  • New functionality includes testing.
    • All tests pass
  • New functionality has been documented.
    • New functionality has javadoc added
  • Commits are signed per the DCO using --signoff
  • Commit changes are listed out in CHANGELOG.md file (See: Changelog)

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.

@kotwanikunal kotwanikunal requested review from a team and reta as code owners November 2, 2022 19:51
@kotwanikunal kotwanikunal force-pushed the searchable-snapshot-integ-tests branch from db495f5 to 1817e04 Compare November 2, 2022 19:52
@github-actions
Copy link
Contributor

github-actions bot commented Nov 2, 2022

Gradle Check (Jenkins) Run Completed with:

@github-actions
Copy link
Contributor

github-actions bot commented Nov 2, 2022

Gradle Check (Jenkins) Run Completed with:

@kotwanikunal kotwanikunal force-pushed the searchable-snapshot-integ-tests branch from 1817e04 to 2b26d2a Compare November 2, 2022 20:14
.setRenamePattern("(.+)")
.setRenameReplacement("$1-copy")
.setStorageType(RestoreSnapshotRequest.StorageType.REMOTE_SNAPSHOT)
.setWaitForCompletion(true)
Copy link
Member

Choose a reason for hiding this comment

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

I'd be inclined to not set "wait for completion" because you assert the relevant state below and it is usually best to avoid making tests wait on timeouts like this if possible

Copy link
Member Author

Choose a reason for hiding this comment

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

Got rid of the synchronous timeout based call.

@@ -118,6 +125,206 @@ public void testCreateSearchableSnapshot() throws Exception {
assertIndexDirectoryDoesNotExist("test-idx-1-copy", "test-idx-2-copy");
}

/**
* Tests the functionality of {@link org.opensearch.cluster.routing.allocation.allocator.RemoteShardsBalancer} to
Copy link
Member

Choose a reason for hiding this comment

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

Nitpick but these types of documentation links tend to go stale with refactorings and it is probably not necessary to link to such low-level code in high-level integration tests like this.

Copy link
Member Author

Choose a reason for hiding this comment

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

Added general statements instead of specific implementation details.

@github-actions
Copy link
Contributor

github-actions bot commented Nov 2, 2022

Gradle Check (Jenkins) Run Completed with:

@kotwanikunal kotwanikunal force-pushed the searchable-snapshot-integ-tests branch from 2b26d2a to e8857ea Compare November 2, 2022 21:38
@kotwanikunal kotwanikunal added backport 2.x Backport to 2.x branch backport 2.4 Backport to 2.4 branch labels Nov 2, 2022
@github-actions
Copy link
Contributor

github-actions bot commented Nov 2, 2022

Gradle Check (Jenkins) Run Completed with:

* nodes with search capabilities are added back to the cluster.
*/
public void testSearchableSnapshotAllocationForFailoverAndRecovery() throws Exception {
final int numReplicasIndex = randomIntBetween(1, 4);
Copy link
Member

Choose a reason for hiding this comment

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

What do you think about taking the randomness away here and setting number of replicas to 1. Then you could run the following scenario:

  1. Ensure green after initial setup
  2. Stop one search node and ensure yellow
  3. Stop another search node and ensure red
  4. Add 3 new search nodes and ensure green
  5. Stop one search node and ensure green

Then I think you could get rid of the "testSearchableSnapshotAllocationWithNoSearchNodes" test case since that is covered here. It would also cover the "automatically recovery from red" scenario that currently isn't being tested.

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 led to a discovery of an issue when the primary remote shards go unassigned. I'll add in the fix for it as well.
Thanks for the suggestion 🙂

@kotwanikunal kotwanikunal force-pushed the searchable-snapshot-integ-tests branch from e8857ea to cb8af5a Compare November 3, 2022 18:04
@github-actions
Copy link
Contributor

github-actions bot commented Nov 3, 2022

Gradle Check (Jenkins) Run Completed with:

@kotwanikunal kotwanikunal force-pushed the searchable-snapshot-integ-tests branch from cb8af5a to 3741502 Compare November 3, 2022 19:59
@github-actions
Copy link
Contributor

github-actions bot commented Nov 3, 2022

Gradle Check (Jenkins) Run Completed with:

@github-actions
Copy link
Contributor

github-actions bot commented Nov 4, 2022

Gradle Check (Jenkins) Run Completed with:

@github-actions
Copy link
Contributor

github-actions bot commented Nov 4, 2022

Gradle Check (Jenkins) Run Completed with:

@andrross andrross removed the backport 2.4 Backport to 2.4 branch label Nov 4, 2022
@kotwanikunal kotwanikunal force-pushed the searchable-snapshot-integ-tests branch from 3741502 to 8f2a00f Compare November 4, 2022 17:04
@kotwanikunal
Copy link
Member Author

This will cause a merge conflict due to #5055. Updating the PR.

@github-actions
Copy link
Contributor

github-actions bot commented Nov 4, 2022

Gradle Check (Jenkins) Run Completed with:

Signed-off-by: Kunal Kotwani <kkotwani@amazon.com>
@kotwanikunal kotwanikunal force-pushed the searchable-snapshot-integ-tests branch from 8f2a00f to 118cca1 Compare November 4, 2022 17:46
@github-actions
Copy link
Contributor

github-actions bot commented Nov 4, 2022

Gradle Check (Jenkins) Run Completed with:

@kotwanikunal kotwanikunal merged commit 082f059 into opensearch-project:main Nov 4, 2022
opensearch-trigger-bot bot pushed a commit that referenced this pull request Nov 4, 2022
Signed-off-by: Kunal Kotwani <kkotwani@amazon.com>

Signed-off-by: Kunal Kotwani <kkotwani@amazon.com>
(cherry picked from commit 082f059)
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
kotwanikunal pushed a commit that referenced this pull request Nov 4, 2022
Signed-off-by: Kunal Kotwani <kkotwani@amazon.com>

Signed-off-by: Kunal Kotwani <kkotwani@amazon.com>
(cherry picked from commit 082f059)
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>

Signed-off-by: Kunal Kotwani <kkotwani@amazon.com>
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
@kotwanikunal kotwanikunal deleted the searchable-snapshot-integ-tests branch January 12, 2023 17:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport 2.x Backport to 2.x branch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants