Cleanup TransportReplicationAction #12395
Conversation
martijnvg
commented
Jul 22, 2015
martijnvg
commented
- Split the actual primary operation from the primary phase into a dedicated AsyncPrimaryAction class (similar to AsyncReplicaAction)
- Removed threaded_operation option from replication based transport actions.
- Let the threading be handled by by the transport service and drop forking from new threads from the transport replication action.
martijnvg
force-pushed
the
cleanup_transport_replication_action
branch
8 times, most recently
from
1a0c33f
to
680e9da
Compare
|
|
||
| }); | ||
| } else { | ||
| if (replicaRequest.operationThreaded()) { |
There was a problem hiding this comment.
so happy that you remove this part!
|
@brwe I've fixed the test and made sure that the newly added |
martijnvg
force-pushed
the
cleanup_transport_replication_action
branch
from
7412f68
to
5a96d4d
Compare
|
I rebased this PR and moved the retry primary handling entirely to the coordinating node. Also I included @brwe test from #12574 which passes with this PR. This test simulates a situation where 2 nodes endlessly redirect same index request to each other caused by a number of events described in #12573. The reason this test is included is because the way primary write requests are redirected has changed in the PR. The coordinating node remains in charge of the entire write operation, even if the primary shard is on a different node than the coordinating node. While before the node that holds the primary shard is always in charge of a write request. In a situation that is described in #12573 this can lead to nodes endlessly redirecting the same write request to each other until the cluster state caught up or something bad happens. With this PR with the #12573 situation, the write request will be retried when a new cluster state comes or the write timeout has been met. |
| try { | ||
| channel.sendResponse(e); | ||
| } catch (Throwable t) { | ||
| logger.warn("failed to send response for get", t); |
|
Overall I really like this split of primary operation and primary phase. I think this might make it much easier to understand what is happening. I left some comments and questions around testing and where we should retry though. Hope they are not too confusing... |
martijnvg
force-pushed
the
cleanup_transport_replication_action
branch
from
5a96d4d
to
dc6c053
Compare
martijnvg
force-pushed
the
cleanup_transport_replication_action
branch
3 times, most recently
from
9c0d06a
to
12db140
Compare
…cated AsyncPrimaryAction class (similar to AsyncReplicaAction) Removed threaded_operation option from replication based transport actions. Let the threading be handled by by the transport service and drop forking from new threads from the transport replication action.
…ccurs but retry on the node holding the primary shard
…ally then use the transport service instead of the threadpool for forking a new thread. This way there is one place where new threads are being spawned.
…ionRequest#internalShardRouting
…uest can get stuck in an endless redirect loop between nodes due to slow cluster state processing.
martijnvg
force-pushed
the
cleanup_transport_replication_action
branch
from
3a9e901
to
3f64076
Compare
|
@bleskes I brought back the chasing of the primary shard to |
| final ShardRouting primary = request.internalShardRouting; | ||
| // Although this gets executed locally, this more of an assertion, but if change the primary action | ||
| // to be performed remotely this check is important to check before performing the action: | ||
| if (clusterService.localNode().id().equals(primary.currentNodeId()) == false) { |
There was a problem hiding this comment.
do we need to do this? if we fail to find the shard in the indicesService, we'll throw an exception anyhow?
There was a problem hiding this comment.
true. it kind of made sense how the pr did work, i left it because imo it would be a nice check. but lets just remove it.
|
The tricky part about the PR is that the cluster state is observed twice, one time in the primary phase and one time in the async primary action. In cases we retry, we might miss the update the to the cluster state. This would only be likely to occur more if the primary action was executed remotely which, is what the plan was after as follow up issue. But this can be dangerous. It is fixable if we remember on what version we decided to execute the primary operation, but that would make this change bigger than the plan is and we should maybe to a break from this change and reconsider it post 2.0 |
