-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
Making shard copy count a multiple of attribute count #3462
Conversation
❌ Gradle Check failure 5870d5aca952b773ac4632ebf44f0093bc3193bb |
❌ Gradle Check failure 7fb9e069ea6c1bdcc109505f33366645630cd018 |
@gbbafna Are you trying to finish this? Care to explain a bit more what this accomplishes above? |
❌ Gradle Check failure 0f3ee95a0fa74f3c764aef072ab98f0855e03575 |
❌ Gradle Check failure db0cc2be50e27d2eafe05a1a8d88cca16610d159 |
❌ Gradle Check failure 870d6dc03ce34ae13537cc45c036cddb04512878 |
1e40a15
to
3328553
Compare
✅ Gradle Check success 332855374dbfcf2924bfb1c990ba1772a57ee0cc |
server/src/main/java/org/opensearch/cluster/routing/allocation/AwarenessReplicaBalance.java
Outdated
Show resolved
Hide resolved
✅ Gradle Check success 41e8297a97cbd3bd96c71675e10e99d19fad49b0 |
server/src/main/java/org/opensearch/cluster/routing/allocation/AwarenessReplicaBalance.java
Show resolved
Hide resolved
int awarenessAttributes = this.awarenessReplicaBalance.awarenessAttributes(); | ||
if (awarenessAttributes > 1) { | ||
if ((settings.getAsInt(IndexMetadata.SETTING_NUMBER_OF_REPLICAS, INDEX_NUMBER_OF_REPLICAS_SETTING.getDefault(Settings.EMPTY)) | ||
+ 1) % awarenessAttributes != 0) { | ||
validationErrors.add("expected total copies needs to be a multiple of total awareness attributes " + awarenessAttributes); | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we also need to mention which attribute zone/rack etc?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point. It will add the complexity of returning attribute as well from awarenessAttributes
function , just for logging purpose. So I prefer not doing it . On seeing the error message with and without the attribute, user would anyways need to revisit awareness attributes values . Do you think this behavior is okay ?
if (awarenessAttributes > 1) { | ||
if ((updatedNumberOfReplicas + 1) % awarenessAttributes != 0) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we put the multiple logic as a Predicate
in the AwarenessReplicaBalance
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done. But we still need to expose awarenessAttributes()
for the validation exception .
...er/src/test/java/org/opensearch/cluster/routing/allocation/AwarenessReplicaBalanceTests.java
Outdated
Show resolved
Hide resolved
✅ Gradle Check success 8b401e5901a19de0bd5c36e7ac0798318e9255aa |
server/src/main/java/org/opensearch/cluster/routing/allocation/AwarenessReplicaBalance.java
Outdated
Show resolved
Hide resolved
server/src/main/java/org/opensearch/cluster/metadata/MetadataUpdateSettingsService.java
Outdated
Show resolved
Hide resolved
server/src/main/java/org/opensearch/cluster/routing/allocation/AwarenessReplicaBalance.java
Outdated
Show resolved
Hide resolved
Gradle Check (Jenkins) Run Completed with:
|
… path Signed-off-by: Gaurav Bafna <gbbafna@amazon.com>
Gradle Check (Jenkins) Run Completed with:
|
Would be great to see rollover tests as well. |
Gradle Check (Jenkins) Run Completed with:
|
Both the failing tests are passing locally and are unrelated to the change here. org.opensearch.cluster.routing.allocation.decider.DiskThresholdDeciderIT.testRestoreSnapshotAllocationDoesNotExceedWatermark |
Gradle Check (Jenkins) Run Completed with:
|
Signed-off-by: Gaurav Bafna <gbbafna@amazon.com>
Gradle Check (Jenkins) Run Completed with:
|
Gradle Check (Jenkins) Run Completed with:
|
The backport to
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
cd .worktrees/backport-2.x
# Create a new branch
git switch --create backport/backport-3462-to-2.x
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 82ff463fd1bc07c241c8674b72182f2875a60425
# Push it to GitHub
git push --set-upstream origin backport/backport-3462-to-2.x
# Go back to the original working tree
cd ../..
# Delete the working tree
git worktree remove .worktrees/backport-2.x Then, create a pull request where the |
…ject#3462) * Making all the copies a multiple of attribute count Signed-off-by: Gaurav Bafna <gbbafna@amazon.com> (cherry picked from commit 82ff463)
…ject#3462) * Making all the copies a multiple of attribute count Signed-off-by: Gaurav Bafna <gbbafna@amazon.com> (cherry picked from commit 82ff463)
Description
A boolean cluster level setting
cluster.routing.allocation.awareness.balance
which is false by default . When true, we would validate that total copies is always a multiple of awareness attribute value count . If not, we will throw a validation exception. If there are multiple awareness attributes, the balance needs to ensure that largest varying attribute of awareness_attribute is equally balance. For ex, if there are 2 Awareness Attributes, zones and rack ids, each having 2 possible values , total copies will to be multiple of 2.Tested
Issues Resolved
#3461
Check List
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.