Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove some abstractions from TransportReplicationAction #40706

Merged
merged 18 commits into from
Apr 3, 2019
Merged

Remove some abstractions from TransportReplicationAction #40706

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
18 commits
Select commit Hold shift + click to select a range
85b1cdb
Use ChannelActionListener in OperationTransportHandler
DaveCTurner Apr 1, 2019
4302320
Generic PrimaryResult
DaveCTurner Apr 1, 2019
16350a6
No need for ShardReference superclass
DaveCTurner Apr 1, 2019
efdab0c
Use ActionListener instead of Channel for primary response handler
DaveCTurner Apr 1, 2019
e1fef41
Use ActionListener instead of Channel for replica response handler
DaveCTurner Apr 1, 2019
4844f9a
Use ConcreteShardRequest throughout AsyncPrimaryAction
DaveCTurner Apr 1, 2019
2135ff9
Use ConcreteReplicaRequest throughout AbstractReplicaAction
DaveCTurner Apr 1, 2019
240edcf
Replace OperationTransportHandler with lambda
DaveCTurner Apr 1, 2019
a076dec
Replace PrimaryOperationTransportHandler with lambda
DaveCTurner Apr 1, 2019
83c6c67
Replace ReplicaOperationTransportHandler with lambda
DaveCTurner Apr 1, 2019
3d7e753
Allow subclasses to control whether primary action is forced
DaveCTurner Apr 1, 2019
df7437c
Imports
DaveCTurner Apr 1, 2019
9f89af7
Revert "Allow subclasses to control whether primary action is forced"
DaveCTurner Apr 1, 2019
a5bfc84
Revert "Revert "Allow subclasses to control whether primary action is…
DaveCTurner Apr 1, 2019
7182fd6
Merge branch 'master' into 2019-04-01-refactor-transport-replication-…
DaveCTurner Apr 2, 2019
fb06813
Rename listener -> onCompletionListener
DaveCTurner Apr 2, 2019
ffc7256
onCompletionListener.onResponse cannot fail
DaveCTurner Apr 2, 2019
c4d9c40
Revert "Revert "Revert "Allow subclasses to control whether primary a…
DaveCTurner Apr 2, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 2 additions & 12 deletions server/src/main/java/org/elasticsearch/action/resync/TransportResyncReplicationAction.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,6 @@
import org.elasticsearch.transport.TransportService;

import java.io.IOException;
import java.util.function.Supplier;

public class TransportResyncReplicationAction extends TransportWriteAction<ResyncReplicationRequest,
ResyncReplicationRequest, ResyncReplicationResponse> implements PrimaryReplicaSyncer.SyncAction {
Expand All @@ -64,18 +63,9 @@ public TransportResyncReplicationAction(Settings settings, TransportService tran
}

@Override
protected void registerRequestHandlers(String actionName, TransportService transportService, Supplier<ResyncReplicationRequest> request,
Supplier<ResyncReplicationRequest> replicaRequest, String executor) {
transportService.registerRequestHandler(actionName, request, ThreadPool.Names.SAME, this::handleOperationRequest);
protected boolean forcePrimaryActionExecution() {
// we should never reject resync because of thread pool capacity on primary
transportService.registerRequestHandler(transportPrimaryAction,
() -> new ConcreteShardRequest<>(request),
executor, true, true,
this::handlePrimaryRequest);
transportService.registerRequestHandler(transportReplicaAction,
() -> new ConcreteReplicaRequest<>(replicaRequest),
executor, true, true,
this::handleReplicaRequest);
return true;
}

@Override
Expand Down
18 changes: 9 additions & 9 deletions ...rc/main/java/org/elasticsearch/action/support/replication/TransportReplicationAction.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -144,21 +144,21 @@ protected TransportReplicationAction(Settings settings, String actionName, Trans

this.transportPrimaryAction = actionName + "[p]";
this.transportReplicaAction = actionName + "[r]";
registerRequestHandlers(actionName, transportService, request, replicaRequest, executor);

transportService.registerRequestHandler(actionName, request, ThreadPool.Names.SAME, this::handleOperationRequest);
transportService.registerRequestHandler(transportPrimaryAction, () -> new ConcreteShardRequest<>(request), executor, true,
forcePrimaryActionExecution(), this::handlePrimaryRequest);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The result of forcePrimaryActionExecution() is passed to the wrong parameter (canTripCircuitBreaker). Swap this and the previous true and I think it will work.

Copy link
Contributor Author

@DaveCTurner DaveCTurner Apr 3, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, well spotted. I opened #40762.

// we must never reject on because of thread pool capacity on replicas
transportService.registerRequestHandler(
transportReplicaAction, () -> new ConcreteReplicaRequest<>(replicaRequest), executor, true, true, this::handleReplicaRequest);

this.transportOptions = transportOptions(settings);

this.syncGlobalCheckpointAfterOperation = syncGlobalCheckpointAfterOperation;
}

protected void registerRequestHandlers(String actionName, TransportService transportService, Supplier<Request> request,
Supplier<ReplicaRequest> replicaRequest, String executor) {
transportService.registerRequestHandler(actionName, request, ThreadPool.Names.SAME, this::handleOperationRequest);
transportService.registerRequestHandler(transportPrimaryAction, () -> new ConcreteShardRequest<>(request), executor,
this::handlePrimaryRequest);
// we must never reject on because of thread pool capacity on replicas
transportService.registerRequestHandler(
transportReplicaAction, () -> new ConcreteReplicaRequest<>(replicaRequest), executor, true, true, this::handleReplicaRequest);
protected boolean forcePrimaryActionExecution() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ideally this would be a constructor parameter instead to avoid the dreaded relying on subclass uninitialized state problem.

However, the old registerRequestHandlers had the same issue, so this is still a good improvement. I would like to add a javadoc comment to it that overriding methods should only return either true and not rely on state.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh darn it I didn't mean to commit that, it doesn't work. At least, I committed it, then reverted it (it doesn't work) but then reverted the reversion to investigate why. I have now reverted the reverted reversion. 🤣

return false;
}

@Override
Expand Down