Add projectsDelta method to ClusterChangedEvent #127697
Conversation
Similar to nodesDelta, the projectsDelta provides a centrailzed method to tell the added and removed projects so that cluster listeners/appliers do not need to compute it individually. Note the delta currently does not have updated. This needs to wait till we stop eagerly creating ProjectMetadata.Builder in Metadata.Builder.
ywangd
added
>non-issue
:Distributed Coordination/Cluster Coordination
elasticsearchmachine
added
the
Team:Distributed Coordination
|
Pinging @elastic/es-distributed-coordination (Team:Distributed Coordination) |
elasticsearchmachine
added
the
serverless-linked
| return new ProjectsDelta( | ||
| Collections.unmodifiableSet(Sets.difference(currentMetadata.projects().keySet(), previousMetadata.projects().keySet())), | ||
| Collections.unmodifiableSet(Sets.difference(previousMetadata.projects().keySet(), currentMetadata.projects().keySet())) | ||
| ); |
There was a problem hiding this comment.
I'd prefer to construct the set differences manually, as we can construct both with one loop if we do it manually. You'll probably think this is a premature optimization, but I think the cost of the optimization is low and it's one less thing to consider optimizing later on.
There was a problem hiding this comment.
+1. Also can we simplify that logic for DEFAULT? It seems looping through them and skipping default project in the loop is more readable.
There was a problem hiding this comment.
I think we still want that shortcut logic for single-project mode to avoid any performance impact in stateful. This wouldn't have a big impact anyway, but every bit counts IMO.
There was a problem hiding this comment.
Updated in fbe4b81. It should be a little more efficient since due to less call to Set#contains and less set constructions. But it essentially still has two 2 loops, 1st to initialize the removed set and 2nd to populate the added set. Please let me know whether it works for you or if you have better ideas.
we still want that shortcut logic for single-project mode
Yeah. I kept that. We don't need to skip the default project in the main loop since it should never be added or removed. I added assertions for it.
There was a problem hiding this comment.
The assertions caused some test failures because they can create unreleasitc cluster state without the default project. Pushed e02d5d5 for fix. Let's see if there are more issues.
There was a problem hiding this comment.
Ah yeah I had something else in mind but I just realized that doesn't work. You're right, we're still essentially looping twice over the data. Then I think I'd prefer your initial solution... 😅 That uses less memory as we need to copy the whole key set here. In the end it's just going to come down to memory vs. runtime, I guess. I think I'd be leaning towards saving some memory to avoid some GC work, but the differences will be minimal, so I'd be fine either way I think.
I think we can also add a shortcut for previousMetadata == currentMetadata, to skip some cluster state updates.
|
|
||
| } | ||
|
|
||
| private static final ProjectsDelta EMPTY_PROJECT_DELTA = new ProjectsDelta(Set.of(), Set.of()); |
There was a problem hiding this comment.
nit: why not move this into the record?
| return new ProjectsDelta( | ||
| Collections.unmodifiableSet(Sets.difference(currentMetadata.projects().keySet(), previousMetadata.projects().keySet())), | ||
| Collections.unmodifiableSet(Sets.difference(previousMetadata.projects().keySet(), currentMetadata.projects().keySet())) | ||
| ); |
There was a problem hiding this comment.
+1. Also can we simplify that logic for DEFAULT? It seems looping through them and skipping default project in the loop is more readable.
Similar to nodesDelta, the projectsDelta provides a centrailzed method to tell the added and removed projects so that cluster listeners/appliers do not need to compute it individually.
Note the delta currently does not have updated. This needs to wait till we stop eagerly creating ProjectMetadata.Builder in Metadata.Builder.