Extract TransportRequestDeduplication from ShardStateAction #37870
Conversation
* Extracted the logic for master request duplication so it can be reused by the snapshotting logic * Removed custom listener used by `ShardStateAction` to not leak these into future users of this class * Changed semantics slightly to get rid of redundant instantiations of the composite listener * Relates elastic#37686
original-brownbear
added
v7.0.0
:Distributed Indexing/Distributed
|
Pinging @elastic/es-distributed |
|
@dnhatn ping (just in case this got lost, no big rush :)) |
|
@original-brownbear sorry for the delay. I will do it today. |
dnhatn
left a comment
dnhatn
left a comment
There was a problem hiding this comment.
Nice change. I left a some nits and a point to discuss.
| /** | ||
| * Register a listener for the given request with the deduplicator. | ||
| * If the request is not yet registered with the deduplicator it will return an {@link ActionListener} | ||
| * that must be completed by the called when the request completes. If the request is already known to |
| * @param listener Listener to invoke on request completion | ||
| * @return Listener that must be invoked by the caller or null when the request is already known | ||
| */ | ||
| public ActionListener<Void> register(T request, ActionListener<Void> listener) { |
There was a problem hiding this comment.
I am making a suggestion but feel free to reject it because for me this PR is very good already. How about adding a new argument Consumer<Listener> that executes the request if it was not executed before (surely we need to rename the method). With this change, I think we can safely use this method without reading the documentation.
There was a problem hiding this comment.
I think we miss a test which ensures that if we register a request multiple times (maybe concurrently), we return a non-null result only once. And we need also to verify that when the returned listener is completed, the cached size is reduced.
There was a problem hiding this comment.
How about adding a new argument Consumer that executes the request if it was not executed before (surely we need to rename the method). With this change, I think we can safely use this method without reading the documentation.
👍 I like it :) On it.
I think we miss a test which ensures that if we register a request multiple times (maybe concurrently), we return a non-null result only once. And we need also to verify that when the returned listener is completed, the cached size is reduced.
Sure, sounds reasonable => on it :)
server/src/main/java/org/elasticsearch/cluster/action/shard/ShardStateAction.java
Outdated
Show resolved
Hide resolved
dnhatn
dismissed
their stale review
I think we miss a test which ensures that if we register a request multiple times (maybe concurrently), we return a non-null result only once. And we need also to verify that when the returned listener is completed, the cached size is reduced.
|
@dnhatn thanks for the review! All points addressed I think :) Take a look and let me know what you think. |
|
All green again :) |
dnhatn
left a comment
dnhatn
left a comment
There was a problem hiding this comment.
I left a nit on the javadocs. This is a nice refactoring. Thanks for an extra iteration @original-brownbear.
server/src/main/java/org/elasticsearch/transport/TransportRequestDeduplicator.java
Outdated
Show resolved
Hide resolved
|
@dnhatn thanks for the review! |
* master: Remove types from watcher docs (elastic#38002) Add test coverage for Painless general casting of boolean and Boolean (elastic#37780) Fixed test bug, lastFollowTime is null if there are no follower indices. Add ECS schema for user-agent ingest processor (elastic#37727) (elastic#37984) Extract TransportRequestDeduplication from ShardStateAction (elastic#37870) Expose retention leases in shard stats (elastic#37991)
* elastic/master: ILM setPriority corrections for a 0 value (elastic#38001) Temporarily disable BWC for retention lease stats (elastic#38049) Skip Shrink when numberOfShards not changed (elastic#37953) Add dispatching to `HandledTransportAction` (elastic#38050) Update httpclient for JDK 11 TLS engine (elastic#37994) Reduce flaxiness of ccr recovery timeouts test (elastic#38035) Fix ILM status to allow unknown fields (elastic#38043) Fix ILM Lifecycle Policy to allow unknown fields (elastic#38041) Update verify repository to allow unknown fields (elastic#37619) [ML] Datafeed deprecation checks (elastic#38026) Deprecate minimum_master_nodes (elastic#37868) Remove types from watcher docs (elastic#38002) Add test coverage for Painless general casting of boolean and Boolean (elastic#37780) Fixed test bug, lastFollowTime is null if there are no follower indices. Add ECS schema for user-agent ingest processor (elastic#37727) (elastic#37984) Extract TransportRequestDeduplication from ShardStateAction (elastic#37870)
…37870) * Extracted the logic for master request duplication so it can be reused by the snapshotting logic * Removed custom listener used by `ShardStateAction` to not leak these into future users of this class * Changed semantics slightly to get rid of redundant instantiations of the composite listener * Relates elastic#37686
…37870) * Extracted the logic for master request duplication so it can be reused by the snapshotting logic * Removed custom listener used by `ShardStateAction` to not leak these into future users of this class * Changed semantics slightly to get rid of redundant instantiations of the composite listener * Relates elastic#37686
…39399) * Extracted the logic for master request duplication so it can be reused by the snapshotting logic * Removed custom listener used by `ShardStateAction` to not leak these into future users of this class * Changed semantics slightly to get rid of redundant instantiations of the composite listener * Relates #37686
4 participants
ShardStateActionto not leak these into future users of this class