Remove TransportSingleCustomOperationAction in favour of TransportSingleShardAction #12350
Conversation
| @@ -123,7 +124,32 @@ private AsyncSingleAction(Request request, ActionListener<Response> listener) { | |||
| if (blockException != null) { | |||
| throw blockException; | |||
| } | |||
| this.shardsIt = shards(clusterState, internalRequest); | |||
| ShardsIterator originalShardsIt = shards(clusterState, internalRequest); | |||
There was a problem hiding this comment.
typically this kind of logic (preferring local shards) is typically folded into the shard iterator. Why do we need a custom implementation here? I know it was there before, but if we're cleaning up, it's a better clean up no?
There was a problem hiding this comment.
Before if preferLocal was true, it would always first try on the local node before trying remote nodes. I assume we want to keep this behaviour?
Before this was implemented by looping over the shard iterator and skipping remote shard routing, if the operation didn't complete successfully on a local node, the shard iterator was reset and it looped again over all the shard routings but no skipping local shard routings.
I agree that it is nicer of this local shard routings first logic is folded into a shard iterator impl.
There was a problem hiding this comment.
but I'm +1 with dropping the custom 'preferring local shard' logic. So lets drop that then?
|
I like the clean up initiative. Left a question about the approach... |
|
@bleskes I left an answer. |
|
Discussed this with @martijnvg and we decided to fold this class into TransportSingleShardAction…
|
martijnvg
force-pushed
the
TransportSingleCustomOperationAction_remove_operation_threaded
branch
from
0ed145f
to
ea45b11
Compare
martijnvg
changed the title
martijnvg
changed the title
|
@bleskes I updated the PR |
| @@ -114,7 +114,7 @@ public String field() { | |||
|
|
|||
| @Override | |||
| public ActionRequestValidationException validate() { | |||
| ActionRequestValidationException validationException = super.validate(); | |||
There was a problem hiding this comment.
can we add a comment why we can't call super?
There was a problem hiding this comment.
on a second thought (and reading further). I think we should change approach here. It's confusing now that this class partially forces an index and partially not. I suggest we make an index in this class consistently optional (mark as nullable, document what it means, remove the validation check). We should also add a utility validateNonNullIndex that does what the current validate does. All the subclasses that want this, can use it.
|
I think this is indeed a better approach. One base class less to maintain. Left some comments... |
|
@bleskes Thanks for looking at this. I updated the PR. |
|
LGTM |
…ingleShardAction to clean up code. The TransportSingleCustomOperationAction `prefer_local` option has been removed as it isn't worth the effort. The TransportSingleShardAction will execute the operation on the receiving node if a concrete list doesn't provide a list of candite shards routings to perform the operation on.
martijnvg
force-pushed
the
TransportSingleCustomOperationAction_remove_operation_threaded
branch
from
da8a819
to
cafc707
Compare
The TransportSingleCustomOperationAction
prefer_localoption has been removed as it isn't worth the effort.The TransportSingleShardAction will execute the operation on the receiving node if a concrete list doesn't provide a list of candite shards routings to perform the operation on.