Enforce external id uniqueness during DesiredNode construction #84227
Conversation
This commit introduces some small refactorings to improve the desired nodes codebase. - DesiredNode must contain a valid external id, otherwise it cannot be built. - DesiredNodes now stores desired nodes as a map that uses desired nodes external id as the key. This fixes a small bug around idempotent updates, as before we were using a list and comparing the desired nodes using that list.
fcofdez
added
>bug
:Distributed Indexing/Distributed
|
Pinging @elastic/es-distributed (Team:Distributed) |
|
Hi @fcofdez, I've created a changelog YAML for you. |
| if (obj == this) return true; | ||
| if (obj == null || obj.getClass() != this.getClass()) return false; | ||
| var that = (DesiredNode) obj; | ||
| return Objects.equals(this.settings, that.settings) |
There was a problem hiding this comment.
I wonder if it is worth noting that a different order of roles will be considered different by this equals. Perhaps it is just me, but it is slightly confusing that we order the roles in the set we extract but then check equals using an unordered roles list in the settings. Not suggesting to change that though, since settings have a certain semantics of arrays, we should keep that.
| } | ||
|
|
||
| private static Map<String, DesiredNode> toMap(final Collection<DesiredNode> desiredNodes) { | ||
| // use a linked hash map to preserve order |
There was a problem hiding this comment.
The order is sligthly confusing. Suppose I PUT two nodes A,B and then afterwards PUT B,A. The second PUT would still say 200 OK, but the order would be A,B.
I suppose we would never do that anyway, so might not be a real problem (same version would use same order client-side I would assume) and the behavior of keeping the original order makes sense. But perhaps we need to elaborate on the order in the comment?
Also, equals on LinkedHashMap does not consider the order, which we rely on for idempotency, worth a comment?
There was a problem hiding this comment.
I've moved to an ordered map based on the desired node external_id so we get a predictable ordering here.
| private final long version; | ||
| private final Map<String, DesiredNode> nodes; | ||
|
|
||
| public DesiredNodes(String historyID, long version, Collection<DesiredNode> nodes) { |
There was a problem hiding this comment.
Should nodes be a list, since we try to preserve order?
| assert memory != null; | ||
| assert storage != null; | ||
| assert version != null; | ||
| if (processors <= 0) { | ||
| throw new IllegalArgumentException("processors must be greater than 0, but got " + processors); | ||
| } | ||
|
|
||
| if (NODE_EXTERNAL_ID_SETTING.get(settings).isBlank()) { |
There was a problem hiding this comment.
I do not think it is an issue, but if we are in a mixed cluster, we risk seeing a desired nodes validated by an old version without an external ID. I should think that the clients we expect to call this will always have a name or an external id specified, so is likely a moot point, but wanted to have your opinion too.
There was a problem hiding this comment.
We were validating this during the settings validation, but we weren't enforcing it in the DesiredNode constructor, so I think we should be good here. See the rest test Test external_id or node.name is required
…arch into desired-nodes-refactor
|
Thanks Henning! |
…n/elasticsearch into datastream-reuse-pipeline-source * 'datastream-reuse-pipeline-source' of github.com:weizijun/elasticsearch: (28 commits) Add JDK 19 to Java testing matrix [ML] add nlp config update serialization tests (elastic#85867) [ML] A text categorization aggregation that works like ML categorization (elastic#80867) [ML] Fix serialisation of text embedding updates (elastic#85863) TSDB: fix wrong initial value of tsidOrd in TimeSeriesIndexSearcher (elastic#85713) Enforce external id uniqueness during DesiredNode construction (elastic#84227) Fix Intellij integration (elastic#85866) Upgrade Azure SDK to version 12.14.4 (elastic#83884) [discovery-gce] Fix initialisation of transport in FIPS mode (elastic#85817) Remove unnecessary docs/changelog/85534.yaml Prevent ThreadContext header leak when sending response (elastic#68649) Add support for impact_areas to health impacts (elastic#85830) Reduce port range re-use in tests (elastic#85777) Fix TranslogTests#testStats (elastic#85828) Remove hppc from cat allocation api (elastic#85842) Fix BuildTests serialization (elastic#85827) Use urgent priority for node shutdown cluster state update (elastic#85838) Remove Task classes from HLRC (elastic#85835) Remove unused migration classes (elastic#85834) Remove uses of Charset name parsing (elastic#85795) ...
This commit introduces some small refactorings to improve the desired
nodes codebase.
be built.
nodes external id as the key. This fixes a small bug around
idempotent updates, as before we were using a list and comparing
the desired nodes using that list.