Remove unnecessary CopyOnWriteHashMap class #83040
Conversation
romseygeek
added
:Core/Infra/Core
|
Pinging @elastic/es-core-infra (Team:Core/Infra) |
| updatedFollowers.putAll(newAutoFollowers); | ||
| for (String removedCluster : removedRemoteClusters) { | ||
| updatedFollowers.remove(removedCluster); | ||
| } |
There was a problem hiding this comment.
you could do updatedFollowers.keySet().removeAll(removedRemoteClusters)?
There was a problem hiding this comment.
The set supports element removal, which removes the corresponding mapping from the map
Huh, I didn't know that about keySet(), nice.
There was a problem hiding this comment.
IntelliJ says that removeAll() is likely to be slow and instead suggests removedRemoteClusters.forEach(updatedFollowers.keySet()::remove);
There was a problem hiding this comment.
Oh, I didn't know about this. This makes me wonder what removeAll is doing under the hood so that it's slower than this naive implementation.
There was a problem hiding this comment.
https://stackoverflow.com/questions/28671903/the-hashsett-removeall-method-is-surprisingly-slow
Looks like it's for the specific case where the collection passed to remove is a List, and the List is larger than the set being modified; removeAll() ends up calling collection.contains() on the List, which is a linear scan. So it probably wouldn't bite us here (the list of clusters to remove will always be smaller than the existing list) but I don't think the forEach impl is any less easy to read and it will stop IDEs shouting at us.
There was a problem hiding this comment.
LGTM
I didn't know auto follow coordinator was the only user of this class. I think back when this was developed, I used this class, because it existed and there were no jdk alternatives (java 8).
I don't suspect this change to have a negative runtime effect on CCR's auto following capability. I expect the performance between the two immutable maps to be similar (but I've not verified this).
|
@elasticmachine update branch |
* upstream/master: (762 commits) [DOCS] Add note to that log4j customization is outside the support scope (elastic#82668) Batch Index Settings Update Requests (elastic#82896) [DOCS] Delete pipeline containing stored script (elastic#83102) Try again to fix changelog areas after reorg (elastic#83100) Bind to non-localhost for transport in some cases (elastic#82973) [DOCS] Reuse multi-level `join` warning (elastic#82976) Remove unnecessary CopyOnWriteHashMap class (elastic#83040) Adjust changelog categories after reorg (elastic#83087) [DOCS] Fix typo in `action.destructive_requires_name` breaking change (elastic#83085) Stack Monitoring: Add Enterprise Search monitoring index templates (elastic#82743) [DOCS] Fix stored script example snippet (elastic#83056) [DOCS] Re-add network traffic para to `term` query (elastic#83047) [DOCS] Rename example stored script (elastic#83054) [ML][DOCS] Add Trained model APIs to the REST APIs index (elastic#82791) [ML] Update running process when global calendar changes (elastic#83044) [Transform] Fix condition on which the transform stops processing buckets (elastic#82852) [DOCS] Fixes field names in ML sum functions. (elastic#83048) [ML] fix NLP tokenization never_split handling around punctuation (elastic#82982) Construct dynamic updates directly via object builders (elastic#81449) Emit trace.id into audit logs (elastic#82849) ... # Conflicts: # client/rest-high-level/src/test/java/org/elasticsearch/client/IndicesClientIT.java # client/rest-high-level/src/test/java/org/elasticsearch/client/documentation/ILMDocumentationIT.java # server/src/main/java/org/elasticsearch/action/admin/indices/rollover/Condition.java # server/src/test/java/org/elasticsearch/action/admin/indices/rollover/ConditionTests.java # x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/ilm/RolloverActionTests.java # x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/ilm/TimeseriesLifecycleTypeTests.java # x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/ilm/WaitForRolloverReadyStepTests.java
This is only used in one class, and can easily be replaced by standard Java
maps.