Add ClusterStateSupplier interface + standard implementation #115931
Conversation
ldematte
added
>non-issue
:Core/Infra/Core
|
Pinging @elastic/es-core-infra (Team:Core/Infra) |
server/src/main/java/org/elasticsearch/cluster/ClusterStateSupplier.java
Outdated
Show resolved
Hide resolved
server/src/main/java/org/elasticsearch/cluster/SafeClusterStateSupplier.java
Outdated
Show resolved
Hide resolved
|
@elasticmachine update branch |
| @Override | ||
| public void clusterChanged(ClusterChangedEvent event) { | ||
| if (isInitialized() || event.state().blocks().hasGlobalBlock(STATE_NOT_RECOVERED_BLOCK) == false) { | ||
| currentClusterState = event.state(); |
There was a problem hiding this comment.
I'm not sure I quite grasp this if condition. Perhaps a comment would help?
It looks like once we've received the first event, we start ignoring STATE_NOT_RECOVERED_BLOCK. Why is that?
There was a problem hiding this comment.
That's the purpose. We just care about the initial recovery; after that, we consider the cluster state as available.
There was a problem hiding this comment.
Ok. But can STATE_NOT_RECOVERED_BLOCK happen again later, after initialization?
- If so, probably this code could use a comment explaining why we want to keep accepting new cluster states in that case but not during initialization.
- If not, then we can get rid of the
isInitializedcall.
There was a problem hiding this comment.
Yes it can, but we care only if the initial recovery is happened yet. I'll add a comment about that.
In general, accessing ClusterState from ClusterService directly (ClusterService#state()) is not safe, as we can't know if the state has been initialized yet. Especially if we start reading it very early (e.g. in a ctor). |
|
Note to self: |
5 participants
This came up while working on serverless; we needed a "safe" way to access ClusterState, avoiding to read from it too early, with a fallback.
We saw this could be useful in more cases, so moving to server made sense.
The default implementation (which we expect to be used in most cases) implements "ready" as "is cluster state available", after the initial recovery.
I left out any binding from it; possibly, under the new DI, we might want to merge the implementation with ClusterService/add the implementation as a default listener and bind it to ClusterStateSupplier.