Fix the bug that masterOperation(with task param) is bypassed (#4103)

The method `protected void masterOperation(Task task, Request request, ClusterState state, ActionListener<Response> listener)` was not properly deprecated by the commit 2a1b239 / PR #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<Response> 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 #4103 (comment)
- Add a test to validate the backwards compatibility, which copied and modified from an existing test.

Signed-off-by: Tianli Feng <[email protected]>

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<Response> 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<Response> 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<Response> listener)
         throws Exception {
-        clusterManagerOperation(request, state, listener);
+        masterOperation(task, request, state, listener);
     }
 
     protected boolean localExecute(Request request) {

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,

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<Response> 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<Response> 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();

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();
     }