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

Parameter improvements to Cluster Health API wait for shards #20223

Merged
merged 3 commits into from
Aug 31, 2016

Conversation

abeyad
Copy link

@abeyad abeyad commented Aug 30, 2016

Previously, the cluster health API used a strictly numeric value for wait_for_active_shards. However, with the introduction of ActiveShardCount and the removal of write consistency level for replication operations, wait_for_active_shards is used for write operations to represent values for ActiveShardCount. This PR moves the cluster health API's usage of wait_for_active_shards to be consistent with its usage in the write operation APIs.

This PR also changes wait_for_relocating_shards from a numeric value to a simple boolean value wait_for_no_relocating_shards to set whether the cluster health operation should wait for
all relocating shards to complete relocation.

Breaking changes summary:

  1. wait_for_active_shards: this is not a breaking change because the same numeric values that were allowed before are allowed now. We have just added the ability to specify all as well, in conformity with the the write operation APIs that also use wait_for_active_shards
  2. wait_for_no_relocating_shards: this is a breaking change because it differs from the old name wait_for_relocating_shards and it takes a boolean value to represent whether we should wait for the cluster to have no relocations, whereas the old wait_for_relocating_shards took a number to represent how many relocating shards to wait on (if this value was set, it was pretty much always set to 0)

Closes #20216

Previously, the cluster health API used a strictly numeric value
for `wait_for_active_shards`. However, with the introduction of
ActiveShardCount and the removal of write consistency level for
replication operations, `wait_for_active_shards` is used for
write operations to represent values for ActiveShardCount. This
commit moves the cluster health API's usage of `wait_for_active_shards`
to be consistent with its usage in the write operation APIs.

This commit also changes `wait_for_relocating_shards` from a
numeric value to a simple boolean value `wait_for_no_relocating_shards`
to set whether the cluster health operation should wait for
all relocating shards to complete relocation.
@abeyad
Copy link
Author

abeyad commented Aug 30, 2016

@elasticmachine retest this please

this.waitForRelocatingShards = waitForRelocatingShards;
/**
* Sets whether the request should wait for there to be no relocating shards before
* retrieving the cluster health status. Defaults to <code>false</code>, meaning the
Copy link
Member

Choose a reason for hiding this comment

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

{@code false}?

@nik9000
Copy link
Member

nik9000 commented Aug 30, 2016

Left a few minor things, mostly documentation wording suggestions. LGTM.

@abeyad
Copy link
Author

abeyad commented Aug 30, 2016

@nik9000 thanks for the review! i pushed b50e472 that addresses your comments

@nik9000
Copy link
Member

nik9000 commented Aug 30, 2016

LGTM

@abeyad abeyad removed the review label Aug 30, 2016
@clintongormley
Copy link
Contributor

I'm OK with this change, but I would add a check for wait_for_relocating_shards and throw an exception if present, otherwise this parameter will just be silently ignored.

it'd be better to make REST param parsing strict (#14719) rather than dealing with these on a case-by-case basis

@abeyad
Copy link
Author

abeyad commented Aug 31, 2016

@clintongormley I pushed a commit to check for the old param and throw an exception: e4e946b

I agree that it would be good to have a general solution to making REST param parsing strict

@abeyad abeyad merged commit 4641254 into elastic:master Aug 31, 2016
@abeyad abeyad deleted the cluster-health-api-wait-count branch August 31, 2016 15:58
@niemyjski
Copy link
Contributor

So in our use case when we create an index we want to block client calls until all shards are created but we don't care about replicas. So I guess with this change I would need to specify all. Personally, I'd like if it was more like cluster heath api and I could just wait for yellow as shard size might not be known... But all works for me.

@abeyad
Copy link
Author

abeyad commented Sep 30, 2016

@niemyjski you wouldn't need to specify anything, because in 5.0, by default index creation will not return until all of the primary shards are active (see #18985).

Suppose you had 2 replicas in addition to your primary. If you wanted to wait for at least 1 replica to become active in addition to the primaries before proceeding with the write, then you would use wait_for_active_shards=2 (1 for primary plus 1 for replica). If you wanted to wait for all replicas, you would specify wait_for_active_shards=3 or more easier wait_for_active_shards=all

@niemyjski
Copy link
Contributor

Awesome! I didn't get that from reading the docs, that's good to know!

@bleskes
Copy link
Contributor

bleskes commented Oct 3, 2016

Awesome! I didn't get that from reading the docs, that's good to know!

@niemyjski if you have any suggestion on how to improve the docs, please let us know (just click that edit button on the website :))

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