From fb2362d23dcfda29e0723cd5fde2cca52477ca05 Mon Sep 17 00:00:00 2001 From: Tianli Feng Date: Wed, 3 Aug 2022 12:16:51 -0700 Subject: [PATCH] Fix the bug that masterOperation(with task param) is bypassed (#4103) The method `protected void masterOperation(Task task, Request request, ClusterState state, ActionListener listener)` was not properly deprecated by the commit https://github.com/opensearch-project/OpenSearch/commit/2a1b239c2af8d7f333a404c66467a50dc0a52c80 / PR https://github.com/opensearch-project/OpenSearch/pull/403. There is a problem that it doesn't make any difference to whether overriding the method `void masterOperation(Task task, Request request, ClusterState state, ActionListener listener)` or not. The reason is the only usage for the method changed to call another method `clusterManagerOperation(task, request, clusterState, l)`, which is the default implementation for the method `masterOperation(with task param)`. There is no other usage for the method `masterOperation()` so it's bypassed. In the commit: - Change delegation model for method `clusterManagerOperation(with task param)` to call `masterOperation(with task param)`, according to the comment below https://github.com/opensearch-project/OpenSearch/pull/4103#discussion_r936946699 - Add a test to validate the backwards compatibility, which copied and modified from an existing test. Signed-off-by: Tianli Feng (cherry picked from commit 3ab0d1f398e0b22b90237a6dd8db97e6376530dc) --- .../TransportClusterManagerNodeAction.java | 11 ++++-- .../info/TransportClusterInfoAction.java | 1 + ...ransportClusterManagerNodeActionTests.java | 37 +++++++++++++++++++ .../java/org/opensearch/test/TestCluster.java | 1 + 4 files changed, 47 insertions(+), 3 deletions(-) diff --git a/server/src/main/java/org/opensearch/action/support/clustermanager/TransportClusterManagerNodeAction.java b/server/src/main/java/org/opensearch/action/support/clustermanager/TransportClusterManagerNodeAction.java index d65ba3eddf776..a97f4ffe555b6 100644 --- a/server/src/main/java/org/opensearch/action/support/clustermanager/TransportClusterManagerNodeAction.java +++ b/server/src/main/java/org/opensearch/action/support/clustermanager/TransportClusterManagerNodeAction.java @@ -125,22 +125,27 @@ protected void masterOperation(Request request, ClusterState state, ActionListen throw new UnsupportedOperationException("Must be overridden"); } + // TODO: Add abstract keyword after removing the deprecated masterOperation() protected void clusterManagerOperation(Request request, ClusterState state, ActionListener listener) throws Exception { masterOperation(request, state, listener); } - /** @deprecated As of 2.2, because supporting inclusive language, replaced by {@link #clusterManagerOperation(Task, ClusterManagerNodeRequest, ClusterState, ActionListener)} */ + /** + * Override this operation if access to the task parameter is needed + * @deprecated As of 2.2, because supporting inclusive language, replaced by {@link #clusterManagerOperation(Task, ClusterManagerNodeRequest, ClusterState, ActionListener)} + */ @Deprecated protected void masterOperation(Task task, Request request, ClusterState state, ActionListener listener) throws Exception { - clusterManagerOperation(task, request, state, listener); + clusterManagerOperation(request, state, listener); } /** * Override this operation if access to the task parameter is needed */ + // TODO: Change the implementation to call 'clusterManagerOperation(request...)' after removing the deprecated masterOperation() protected void clusterManagerOperation(Task task, Request request, ClusterState state, ActionListener listener) throws Exception { - clusterManagerOperation(request, state, listener); + masterOperation(task, request, state, listener); } protected boolean localExecute(Request request) { diff --git a/server/src/main/java/org/opensearch/action/support/clustermanager/info/TransportClusterInfoAction.java b/server/src/main/java/org/opensearch/action/support/clustermanager/info/TransportClusterInfoAction.java index 1411ff7b30695..c43256a61e8b4 100644 --- a/server/src/main/java/org/opensearch/action/support/clustermanager/info/TransportClusterInfoAction.java +++ b/server/src/main/java/org/opensearch/action/support/clustermanager/info/TransportClusterInfoAction.java @@ -88,6 +88,7 @@ protected final void clusterManagerOperation(final Request request, final Cluste doClusterManagerOperation(request, concreteIndices, state, listener); } + // TODO: Add abstract keyword after removing the deprecated doMasterOperation() protected void doClusterManagerOperation( Request request, String[] concreteIndices, diff --git a/server/src/test/java/org/opensearch/action/support/clustermanager/TransportClusterManagerNodeActionTests.java b/server/src/test/java/org/opensearch/action/support/clustermanager/TransportClusterManagerNodeActionTests.java index 9ec9e656257d6..1195ed2590b1e 100644 --- a/server/src/test/java/org/opensearch/action/support/clustermanager/TransportClusterManagerNodeActionTests.java +++ b/server/src/test/java/org/opensearch/action/support/clustermanager/TransportClusterManagerNodeActionTests.java @@ -274,6 +274,43 @@ protected void clusterManagerOperation(Task task, Request request, ClusterState } } + /* The test is copied from testLocalOperationWithoutBlocks() + to validate the backwards compatibility for the deprecated method masterOperation(with task parameter). */ + public void testDeprecatedMasterOperationWithTaskParameterCanBeCalled() throws ExecutionException, InterruptedException { + final boolean clusterManagerOperationFailure = randomBoolean(); + + Request request = new Request(); + PlainActionFuture listener = new PlainActionFuture<>(); + + final Exception exception = new Exception(); + final Response response = new Response(); + + setState(clusterService, ClusterStateCreationUtils.state(localNode, localNode, allNodes)); + + new Action("internal:testAction", transportService, clusterService, threadPool) { + @Override + protected void masterOperation(Task task, Request request, ClusterState state, ActionListener listener) { + if (clusterManagerOperationFailure) { + listener.onFailure(exception); + } else { + listener.onResponse(response); + } + } + }.execute(request, listener); + assertTrue(listener.isDone()); + + if (clusterManagerOperationFailure) { + try { + listener.get(); + fail("Expected exception but returned proper result"); + } catch (ExecutionException ex) { + assertThat(ex.getCause(), equalTo(exception)); + } + } else { + assertThat(listener.get(), equalTo(response)); + } + } + public void testLocalOperationWithBlocks() throws ExecutionException, InterruptedException { final boolean retryableBlock = randomBoolean(); final boolean unblockBeforeTimeout = randomBoolean(); diff --git a/test/framework/src/main/java/org/opensearch/test/TestCluster.java b/test/framework/src/main/java/org/opensearch/test/TestCluster.java index c2e90f0369e6c..478b692fb06ef 100644 --- a/test/framework/src/main/java/org/opensearch/test/TestCluster.java +++ b/test/framework/src/main/java/org/opensearch/test/TestCluster.java @@ -127,6 +127,7 @@ public void assertAfterTest() throws Exception { /** * Returns the number of data and cluster-manager eligible nodes in the cluster. */ + // TODO: Add abstract keyword after removing the deprecated numDataAndMasterNodes() public int numDataAndClusterManagerNodes() { return numDataAndMasterNodes(); }