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

Treat ack timeout more like a publish timeout #31303

Merged
merged 5 commits into from
Jun 14, 2018

Conversation

ywelsch
Copy link
Contributor

@ywelsch ywelsch commented Jun 13, 2018

This PR changes the ack timeout mechanism so that its behavior is closer to the publish timeout, i.e., it only comes into play after committing a cluster state. This ensures for example that an index creation request with a low (ack) timeout value does not return before the cluster state that contains information about the newly created index is even committed.

@ywelsch ywelsch added >enhancement v7.0.0 :Distributed/Cluster Coordination Cluster formation and cluster state publication, including cluster membership and fault detection. v6.4.0 labels Jun 13, 2018
@ywelsch ywelsch requested a review from bleskes June 13, 2018 15:34
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-distributed

Copy link
Contributor

@bleskes bleskes left a comment

Choose a reason for hiding this comment

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

LGTM. Left some minor suggestions.

@@ -48,6 +49,7 @@
void publish(ClusterChangedEvent clusterChangedEvent, AckListener ackListener);

interface AckListener {
void onCommit(TimeValue commitTime);
Copy link
Contributor

Choose a reason for hiding this comment

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

java docs please on what the commitTime means/

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed in 087518e

}

@Override
public TimeValue timeout() {
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if we want to rename this one to avoid confusion.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure about this one. Currently this is defined in ClusterStateTaskConfig. The confusion only comes when we mix the ClusterStateTaskConfig with the AckedClusterStateTaskListener into AckedClusterStateUpdateTask. Mixing all this into one class might be the trappy thing here, not the naming.


@Override
public void clusterStateProcessed(String source, ClusterState oldState, ClusterState newState) {
// ok
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we we want to verify we ended up here? i.e., cluster state was committed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed in 08f15fb

@ywelsch ywelsch merged commit 8f886cd into elastic:master Jun 14, 2018
@ywelsch
Copy link
Contributor Author

ywelsch commented Jun 14, 2018

Thanks @bleskes

ywelsch added a commit that referenced this pull request Jun 14, 2018
This commit changes the ack timeout mechanism so that its behavior is closer to the publish
timeout, i.e., it only comes into play after committing a cluster state. This ensures for example that
an index creation request with a low (ack) timeout value does not return before the cluster state
that contains information about the newly created index is even committed.
tlrx added a commit that referenced this pull request Jun 15, 2018
* master:
  992c788 Uncouple persistent task state and status (#31031)
  8c6ee7d Describe how to add a plugin in Dockerfile (#31340)
  1c5cec0 Remove http status code maps (#31350)
  87a676e Do not set vm.max_map_count when unnecessary (#31285)
  e5b7137 TEST: getCapturedRequestsAndClear should be atomic (#31312)
  0324103 Painless: Fix bug for static method calls on interfaces (#31348)
  d6d0727 QA: Fix resolution of default distribution (#31351)
  fcf1e41 Extract common http logic to server (#31311)
  6dd81ea Build: Fix the license in the pom zip and tar (#31336)
  8f886cd Treat ack timeout more like a publish timeout (#31303)
  9b29327 [ML] Add description to ML filters (#31330)
  f7a0caf SQL: Fix build on Java 10
  375d09c [TEST] Fix RemoteClusterClientTests#testEnsureWeReconnect
  4877cec More detailed tracing when writing metadata (#31319)
  bbfe1ec [Tests] Mutualize fixtures code in BaseHttpFixture (#31210)
tlrx added a commit that referenced this pull request Jun 15, 2018
* 6.x:
  6d711fa (origin/6.x, 6.x) Uncouple persistent task state and status (#31031)
  f0f16b7 [TEST] Make SSL restrictions update atomic (#31050)
  652193f Describe how to add a plugin in Dockerfile (#31340)
  bb568ab TEST: getCapturedRequestsAndClear should be atomic (#31312)
  6f94914 Do not set vm.max_map_count when unnecessary (#31285)
  c12f3c7 Painless: Fix bug for static method calls on interfaces (#31348)
  21128e2 QA: Fix resolution of default distribution (#31351)
  df17a83 Build: Fix the license in the pom zip and tar (#31336)
  3e76b15 Treat ack timeout more like a publish timeout (#31303)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:Distributed/Cluster Coordination Cluster formation and cluster state publication, including cluster membership and fault detection. >enhancement v6.4.0 v7.0.0-beta1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants