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 max_shard_size parameter for Shrink API #5229

Merged
merged 10 commits into from
Nov 30, 2022

Conversation

gaobinlong
Copy link
Collaborator

@gaobinlong gaobinlong commented Nov 12, 2022

Signed-off-by: Gao Binlong [email protected]

Description

As the document mentions that we have a parameter max_primary_shard_size, but actually it doesn't exist, and the parameter is used to generate an optimum number_of_shards of the new shrunken index, so it's useful for users. I've implemented the function and changed the parameter's name to max_shard_size so that it can be consistent with the similar function in Shrink Action in Index Management Plugin.

The main changes of this PR are:

  • Add max_shard_size parameter for Shrink API.
  • Add max_shard_size parameter for Shrink API in high level rest client.
  • Add unit test, integration test and yaml rest test.

Issues Resolved

#5170

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.

@gaobinlong gaobinlong requested review from a team and reta as code owners November 12, 2022 09:55
Signed-off-by: Gao Binlong <[email protected]>
@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

Signed-off-by: Gao Binlong <[email protected]>
@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

@codecov-commenter
Copy link

codecov-commenter commented Nov 13, 2022

Codecov Report

Merging #5229 (a1214a6) into main (110e409) will decrease coverage by 0.04%.
The diff coverage is 64.10%.

@@             Coverage Diff              @@
##               main    #5229      +/-   ##
============================================
- Coverage     71.02%   70.98%   -0.05%     
- Complexity    58119    58158      +39     
============================================
  Files          4711     4711              
  Lines        277533   277571      +38     
  Branches      40169    40180      +11     
============================================
- Hits         197125   197030      -95     
- Misses        64256    64402     +146     
+ Partials      16152    16139      -13     
Impacted Files Coverage Δ
...ion/admin/indices/shrink/ResizeRequestBuilder.java 0.00% <0.00%> (ø)
...rch/action/admin/indices/shrink/ResizeRequest.java 56.62% <35.71%> (-1.35%) ⬇️
...a/org/opensearch/client/indices/ResizeRequest.java 83.78% <66.66%> (-1.52%) ⬇️
...on/admin/indices/shrink/TransportResizeAction.java 65.00% <90.00%> (+5.74%) ⬆️
...adonly/AddIndexBlockClusterStateUpdateRequest.java 0.00% <0.00%> (-75.00%) ⬇️
...readonly/TransportVerifyShardIndexBlockAction.java 9.75% <0.00%> (-73.18%) ⬇️
...n/admin/indices/readonly/AddIndexBlockRequest.java 17.85% <0.00%> (-53.58%) ⬇️
.../java/org/opensearch/node/NodeClosedException.java 50.00% <0.00%> (-50.00%) ⬇️
.../index/shard/IndexShardNotRecoveringException.java 0.00% <0.00%> (-50.00%) ⬇️
...regations/metrics/AbstractHyperLogLogPlusPlus.java 51.72% <0.00%> (-44.83%) ⬇️
... and 453 more

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

@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

@kolchfa-aws
Copy link

When this is done, please open a documentation issue to update the docs. Thanks!

Signed-off-by: Gao Binlong <[email protected]>
@gaobinlong
Copy link
Collaborator Author

@kolchfa-aws, sure, I will do that.

@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

Signed-off-by: Gao Binlong <[email protected]>
Signed-off-by: Gao Binlong <[email protected]>
@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

  • RESULT: UNSTABLE ❕
  • TEST FAILURES:
      2 org.opensearch.index.ShardIndexingPressureConcurrentExecutionTests.testCoordinatingPrimaryThreadedUpdateToShardLimitsAndRejections
      1 org.opensearch.snapshots.DedicatedClusterSnapshotRestoreIT.testIndexDeletionDuringSnapshotCreationInQueue

@gaobinlong
Copy link
Collaborator Author

@reta hi, I've made some change after your last review, can you help to take a look? Thanks.

@reta
Copy link
Collaborator

reta commented Nov 25, 2022

@gaobinlong could you please create an issue at https://github.com/opensearch-project/documentation-website to document this new parameter for Shrink API, thank you

Copy link
Member

@dblock dblock left a comment

Choose a reason for hiding this comment

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

Tiny nit in CHANGELOG, let's keep that clean pls.

Also what's the behavior without max shard size? No limit?

CHANGELOG.md Outdated
@@ -84,6 +84,8 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/),
### Added
- Prevent deletion of snapshots that are backing searchable snapshot indexes ([#5069](https://github.com/opensearch-project/OpenSearch/pull/5069))

Copy link
Member

@dblock dblock Nov 28, 2022

Choose a reason for hiding this comment

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

Delete the new blank line above please.

@@ -702,6 +703,9 @@ private void resizeTest(ResizeType resizeType, CheckedFunction<ResizeRequest, Re
if (resizeType == ResizeType.SPLIT) {
resizeRequest.setSettings(Settings.builder().put("index.number_of_shards", 2).build());
}
if (resizeType == ResizeType.SHRINK) {
Copy link
Member

Choose a reason for hiding this comment

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

Nit: add else

CHANGELOG.md Outdated
@@ -84,6 +84,8 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/),
### Added
- Prevent deletion of snapshots that are backing searchable snapshot indexes ([#5069](https://github.com/opensearch-project/OpenSearch/pull/5069))

- Add max_shard_size parameter for Shrink API ([#5229](https://github.com/opensearch-project/OpenSearch/pull/5229))
Copy link
Member

Choose a reason for hiding this comment

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

Lowercase shrink

@gaobinlong
Copy link
Collaborator Author

@reta I've opened a document issue yet, thanks again for your review.

@gaobinlong
Copy link
Collaborator Author

@dblock thanks for your review, I have made some changes yet. Without max_shard_size, there is no limit on shard's storage size, but there is a document count limit for every shard and the upper limit is 2^31-1-128:
https://github.com/apache/lucene/blob/0cc6f695363419ab0f89e2bef5e7595ace077345/lucene/core/src/java/org/apache/lucene/index/IndexWriter.java#L200).
And we will throw illegalArgumentException if the new shard's document count will exceed the upper limit.

@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

@dblock dblock merged commit 953a3d6 into opensearch-project:main Nov 30, 2022
@dblock dblock added the backport 2.x Backport to 2.x branch label Nov 30, 2022
@opensearch-trigger-bot
Copy link
Contributor

The backport to 2.x failed:

The process '/usr/bin/git' failed with exit code 128

To backport manually, run these commands in your terminal:

# Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add ../.worktrees/backport-2.x 2.x
# Navigate to the new working tree
pushd ../.worktrees/backport-2.x
# Create a new branch
git switch --create backport/backport-5229-to-2.x
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 953a3d6851632c9b09c8dc15dbff02c8aafac6c1
# Push it to GitHub
git push --set-upstream origin backport/backport-5229-to-2.x
# Go back to the original working tree
popd
# Delete the working tree
git worktree remove ../.worktrees/backport-2.x

Then, create a pull request where the base branch is 2.x and the compare/head branch is backport/backport-5229-to-2.x.

@dblock
Copy link
Member

dblock commented Nov 30, 2022

@gaobinlong Looks like it will need a manual backport to 2.x, thx.

@gaobinlong
Copy link
Collaborator Author

@dblock ok, I will do it.

gaobinlong added a commit to gaobinlong/OpenSearch that referenced this pull request Dec 9, 2022
* Add max_shard_size parameter for Shrink API

Signed-off-by: Gao Binlong <[email protected]>

* add change log

Signed-off-by: Gao Binlong <[email protected]>

* fix yaml test failed

Signed-off-by: Gao Binlong <[email protected]>

* optimize the code

Signed-off-by: Gao Binlong <[email protected]>

* fix test failed

Signed-off-by: Gao Binlong <[email protected]>

* optimize changelog & code

Signed-off-by: Gao Binlong <[email protected]>

Signed-off-by: Gao Binlong <[email protected]>
(cherry picked from commit 953a3d6)
reta pushed a commit that referenced this pull request Dec 9, 2022
* Add max_shard_size parameter for Shrink API

Signed-off-by: Gao Binlong <[email protected]>

* add change log

Signed-off-by: Gao Binlong <[email protected]>

* fix yaml test failed

Signed-off-by: Gao Binlong <[email protected]>

* optimize the code

Signed-off-by: Gao Binlong <[email protected]>

* fix test failed

Signed-off-by: Gao Binlong <[email protected]>

* optimize changelog & code

Signed-off-by: Gao Binlong <[email protected]>

Signed-off-by: Gao Binlong <[email protected]>
(cherry picked from commit 953a3d6)
@reta reta mentioned this pull request Jul 17, 2024
3 tasks
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.

5 participants