Observe cluster state on health request #8350
Conversation
|
@bleskes here you go :) |
| } | ||
| if (System.currentTimeMillis() > endTime) { |
There was a problem hiding this comment.
The first line of the method can be removed now - the endTime variable is not used.
long endTime = System.currentTimeMillis() + request.timeout().millis();
There was a problem hiding this comment.
I moved this over to be entirely non-blocking now...
s1monw
force-pushed
the
cluster_health_on_update
branch
from
c25bef8
to
b2c8288
Compare
|
@bleskes I updated the PR as we discussed |
| @@ -241,15 +240,15 @@ public void onTimeout(TimeValue timeout) { | |||
| } | |||
| } | |||
|
|
|||
| public interface Listener { | |||
| public static abstract class Listener { | |||
There was a problem hiding this comment.
I don't think there is an added value to have this as an abstract class anymore, right?
|
pushed another round |
| @@ -90,14 +86,15 @@ public ClusterState execute(ClusterState currentState) { | |||
|
|
|||
| @Override | |||
| public void clusterStateProcessed(String source, ClusterState oldState, ClusterState newState) { | |||
| latch.countDown(); | |||
| TimeValue newTimeout = TimeValue.timeValueMillis(Math.max(0, endTime - System.currentTimeMillis())); | |||
There was a problem hiding this comment.
I think a time out of 0 is problematic in the InternalClusterService logic. We should protect for it, either in executeHealth or in the ClusterStateObserver
There was a problem hiding this comment.
hmm but can't you set this via the API already?
There was a problem hiding this comment.
I double checked and the scheduler can take a value of 0 and the listener removal logic is also OK where postAdded and onTimeout are called in rapid succession. I still think the check for 0 here is clearer. Thanks for adding it.
|
pushed a check for |
|
Agreed. LGTM - thanks for cleaning this up. |
Today we use busy waiting and sampling when we execute HealthReqeusts on the master. This is tricky sicne we might sample a not yet fully applied cluster state and make a decsions base on the partial cluster state. This can lead to ugly problems since requests might be routed to nodes where shards are already marked as relocated but on the actual cluster state they are still started. Yet, this window is very small usually it can lead to ugly test failures. This commit moves the health request over to a listener pattern that gets the actual applied cluster state. Closes elastic#8350
s1monw
force-pushed
the
cluster_health_on_update
branch
from
df835d8
to
cc8e8e6
Compare
Today we use busy waiting and sampling when we execute HealthReqeusts on the master. This is tricky sicne we might sample a not yet fully applied cluster state and make a decsions base on the partial cluster state. This can lead to ugly problems since requests might be routed to nodes where shards are already marked as relocated but on the actual cluster state they are still started. Yet, this window is very small usually it can lead to ugly test failures. This commit moves the health request over to a listener pattern that gets the actual applied cluster state. Closes elastic#8350
clintongormley
added
:Core/Infra/Core
clintongormley
changed the title
Today we use busy waiting and sampling when we execute HealthReqeusts
on the master. This is tricky sicne we might sample a not yet fully applied
cluster state and make a decsions base on the partial cluster state. This can
lead to ugly problems since requests might be routed to nodes where shards are
already marked as relocated but on the actual cluster state they are still started.
Yet, this window is very small usually it can lead to ugly test failures.
This commit moves the health request over to a listener pattern that gets the actual
applied cluster state.