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

Projects
None yet
5 participants
@abeyad
Copy link
Contributor

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

Ali Beyad
Params improvements to Cluster Health API wait for shards
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

This comment has been minimized.

Copy link
Contributor Author

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

This comment has been minimized.

Copy link
@nik9000

nik9000 Aug 30, 2016

Contributor

{@code false}?

public ClusterHealthRequest waitForActiveShards(int waitForActiveShards) {
this.waitForActiveShards = waitForActiveShards;
/**
* Sets the number of shard copies that must be active before getting the health status.

This comment has been minimized.

Copy link
@nik9000

nik9000 Aug 30, 2016

Contributor

"Sets the number of shard copies that must be active across all indices before getting the health status"?

This comment has been minimized.

Copy link
@abeyad

abeyad Aug 30, 2016

Author Contributor

done

* Set this value to {@link ActiveShardCount#ALL} to wait for all shards (primary and
* all replicas) to be active across all indices in the cluster. Otherwise, use
* {@link ActiveShardCount#from(int)} to set this value to any non-negative integer, up to the
* total number of shard copies that would exist across all indices in the cluster.

This comment has been minimized.

Copy link
@nik9000

nik9000 Aug 30, 2016

Contributor

s/that would exist across all indices in the cluster./to wait for./?

This comment has been minimized.

Copy link
@abeyad

abeyad Aug 30, 2016

Author Contributor

done

* shard count is passed in, instead of having to first call {@link ActiveShardCount#from(int)}
* to get the ActiveShardCount.
*/
public ClusterHealthRequest waitForActiveShards(final int waitForActiveShards) {

This comment has been minimized.

Copy link
@nik9000

nik9000 Aug 30, 2016

Contributor

You even kept the java api compatible here. Very kind of you.

This comment has been minimized.

Copy link
@abeyad

abeyad Aug 30, 2016

Author Contributor

😀

A number controlling to how many active
shards to wait for. Defaults to not wait.
A number controlling to how many active shards to wait for, or `all` to wait
for all shards in the cluster to be active. Defaults to not wait.

This comment has been minimized.

Copy link
@nik9000

nik9000 Aug 30, 2016

Contributor

"or none to not wait. Defaults to none."?

This comment has been minimized.

Copy link
@abeyad

abeyad Aug 30, 2016

Author Contributor

done

- do:
catch: request_timeout
cluster.health:
timeout: 1s

This comment has been minimized.

Copy link
@nik9000

nik9000 Aug 30, 2016

Contributor

1ms?

@nik9000

This comment has been minimized.

Copy link
Contributor

commented Aug 30, 2016

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

Ali Beyad
@abeyad

This comment has been minimized.

Copy link
Contributor Author

commented Aug 30, 2016

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

@nik9000

This comment has been minimized.

Copy link
Contributor

commented Aug 30, 2016

LGTM

@abeyad abeyad removed the review label Aug 30, 2016

@clintongormley

This comment has been minimized.

Copy link
Member

commented Aug 31, 2016

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

This comment has been minimized.

Copy link
Contributor Author

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

1 of 2 checks passed

elasticsearch-ci Build finished.
Details
CLA Commit author is a member of Elasticsearch
Details

@abeyad abeyad deleted the abeyad:cluster-health-api-wait-count branch Aug 31, 2016

@niemyjski

This comment has been minimized.

Copy link
Contributor

commented Sep 30, 2016

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

This comment has been minimized.

Copy link
Contributor Author

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

This comment has been minimized.

Copy link
Contributor

commented Sep 30, 2016

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

@bleskes

This comment has been minimized.

Copy link
Member

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
You can’t perform that action at this time.