Serialize NodesStatsRequest as set of strings (#53235) (#53313)

* Add unit tests before refactoring

* Convert boolean fields to set of strings

In order to make nodes stats plugins pluggable, we need to make the
NodesStatsRequest class capable of carrying a flexible list of metrics
rather than a fixed list of boolean flags. This commit changes the
internal storage of the class without changing its serialization.

* Change serialization of NodesStatsRequest

* Set up BWC before merging

* Singularize enum name

server/src/main/java/org/elasticsearch/action/admin/cluster/node/stats/NodesStatsRequest.java

@@ -26,6 +26,10 @@ import org.elasticsearch.common.io.stream.StreamInput;
 import org.elasticsearch.common.io.stream.StreamOutput;
 
 import java.io.IOException;
+import java.util.Arrays;
+import java.util.HashSet;
+import java.util.Set;
+import java.util.stream.Collectors;
 
 /**
  * A request to get node (cluster) level stats.
@@ -33,18 +37,7 @@ import java.io.IOException;
 public class NodesStatsRequest extends BaseNodesRequest<NodesStatsRequest> {
 
     private CommonStatsFlags indices = new CommonStatsFlags();
-    private boolean os;
+    private final Set<String> requestedMetrics = new HashSet<>();
-    private boolean process;
-    private boolean jvm;
-    private boolean threadPool;
-    private boolean fs;
-    private boolean transport;
-    private boolean http;
-    private boolean breaker;
-    private boolean script;
-    private boolean discovery;
-    private boolean ingest;
-    private boolean adaptiveSelection;
 
     public NodesStatsRequest() {
         super((String[]) null);
@@ -52,22 +45,26 @@ public class NodesStatsRequest extends BaseNodesRequest<NodesStatsRequest> {
 
     public NodesStatsRequest(StreamInput in) throws IOException {
         super(in);
+
         indices = new CommonStatsFlags(in);
-        os = in.readBoolean();
+        requestedMetrics.clear();
-        process = in.readBoolean();
+        if (in.getVersion().before(Version.V_7_7_0)) {
-        jvm = in.readBoolean();
+            addOrRemoveMetric(in.readBoolean(), Metric.OS.metricName());
-        threadPool = in.readBoolean();
+            addOrRemoveMetric(in.readBoolean(), Metric.PROCESS.metricName());
-        fs = in.readBoolean();
+            addOrRemoveMetric(in.readBoolean(), Metric.JVM.metricName());
-        transport = in.readBoolean();
+            addOrRemoveMetric(in.readBoolean(), Metric.THREAD_POOL.metricName());
-        http = in.readBoolean();
+            addOrRemoveMetric(in.readBoolean(), Metric.FS.metricName());
-        breaker = in.readBoolean();
+            addOrRemoveMetric(in.readBoolean(), Metric.TRANSPORT.metricName());
-        script = in.readBoolean();
+            addOrRemoveMetric(in.readBoolean(), Metric.HTTP.metricName());
-        discovery = in.readBoolean();
+            addOrRemoveMetric(in.readBoolean(), Metric.BREAKER.metricName());
-        ingest = in.readBoolean();
+            addOrRemoveMetric(in.readBoolean(), Metric.SCRIPT.metricName());
-        if (in.getVersion().onOrAfter(Version.V_6_1_0)) {
+            addOrRemoveMetric(in.readBoolean(), Metric.DISCOVERY.metricName());
-            adaptiveSelection = in.readBoolean();
+            addOrRemoveMetric(in.readBoolean(), Metric.INGEST.metricName());
+            if (in.getVersion().onOrAfter(Version.V_6_1_0)) {
+                addOrRemoveMetric(in.readBoolean(), Metric.ADAPTIVE_SELECTION.metricName());
+            }
         } else {
-            adaptiveSelection = false;
+            requestedMetrics.addAll(in.readStringList());
         }
     }
 
@@ -84,18 +81,7 @@ public class NodesStatsRequest extends BaseNodesRequest<NodesStatsRequest> {
      */
     public NodesStatsRequest all() {
         this.indices.all();
-        this.os = true;
+        this.requestedMetrics.addAll(Metric.allMetrics());
-        this.process = true;
-        this.jvm = true;
-        this.threadPool = true;
-        this.fs = true;
-        this.transport = true;
-        this.http = true;
-        this.breaker = true;
-        this.script = true;
-        this.discovery = true;
-        this.ingest = true;
-        this.adaptiveSelection = true;
         return this;
     }
 
@@ -104,18 +90,7 @@ public class NodesStatsRequest extends BaseNodesRequest<NodesStatsRequest> {
      */
     public NodesStatsRequest clear() {
         this.indices.clear();
-        this.os = false;
+        this.requestedMetrics.clear();
-        this.process = false;
-        this.jvm = false;
-        this.threadPool = false;
-        this.fs = false;
-        this.transport = false;
-        this.http = false;
-        this.breaker = false;
-        this.script = false;
-        this.discovery = false;
-        this.ingest = false;
-        this.adaptiveSelection = false;
         return this;
     }
 
@@ -144,14 +119,14 @@ public class NodesStatsRequest extends BaseNodesRequest<NodesStatsRequest> {
      * Should the node OS be returned.
      */
     public boolean os() {
-        return this.os;
+        return Metric.OS.containedIn(requestedMetrics);
     }
 
     /**
      * Should the node OS be returned.
      */
     public NodesStatsRequest os(boolean os) {
-        this.os = os;
+        addOrRemoveMetric(os, Metric.OS.metricName());
         return this;
     }
 
@@ -159,14 +134,14 @@ public class NodesStatsRequest extends BaseNodesRequest<NodesStatsRequest> {
      * Should the node Process be returned.
      */
     public boolean process() {
-        return this.process;
+        return Metric.PROCESS.containedIn(requestedMetrics);
     }
 
     /**
      * Should the node Process be returned.
      */
     public NodesStatsRequest process(boolean process) {
-        this.process = process;
+        addOrRemoveMetric(process, Metric.PROCESS.metricName());
         return this;
     }
 
@@ -174,14 +149,14 @@ public class NodesStatsRequest extends BaseNodesRequest<NodesStatsRequest> {
      * Should the node JVM be returned.
      */
     public boolean jvm() {
-        return this.jvm;
+        return Metric.JVM.containedIn(requestedMetrics);
     }
 
     /**
      * Should the node JVM be returned.
      */
     public NodesStatsRequest jvm(boolean jvm) {
-        this.jvm = jvm;
+        addOrRemoveMetric(jvm, Metric.JVM.metricName());
         return this;
     }
 
@@ -189,14 +164,14 @@ public class NodesStatsRequest extends BaseNodesRequest<NodesStatsRequest> {
      * Should the node Thread Pool be returned.
      */
     public boolean threadPool() {
-        return this.threadPool;
+        return Metric.THREAD_POOL.containedIn(requestedMetrics);
     }
 
     /**
      * Should the node Thread Pool be returned.
      */
     public NodesStatsRequest threadPool(boolean threadPool) {
-        this.threadPool = threadPool;
+        addOrRemoveMetric(threadPool, Metric.THREAD_POOL.metricName());
         return this;
     }
 
@@ -204,14 +179,14 @@ public class NodesStatsRequest extends BaseNodesRequest<NodesStatsRequest> {
      * Should the node file system stats be returned.
      */
     public boolean fs() {
-        return this.fs;
+        return Metric.FS.containedIn(requestedMetrics);
     }
 
     /**
      * Should the node file system stats be returned.
      */
     public NodesStatsRequest fs(boolean fs) {
-        this.fs = fs;
+        addOrRemoveMetric(fs, Metric.FS.metricName());
         return this;
     }
 
@@ -219,14 +194,14 @@ public class NodesStatsRequest extends BaseNodesRequest<NodesStatsRequest> {
      * Should the node Transport be returned.
      */
     public boolean transport() {
-        return this.transport;
+        return Metric.TRANSPORT.containedIn(requestedMetrics);
     }
 
     /**
      * Should the node Transport be returned.
      */
     public NodesStatsRequest transport(boolean transport) {
-        this.transport = transport;
+        addOrRemoveMetric(transport, Metric.TRANSPORT.metricName());
         return this;
     }
 
@@ -234,92 +209,146 @@ public class NodesStatsRequest extends BaseNodesRequest<NodesStatsRequest> {
      * Should the node HTTP be returned.
      */
     public boolean http() {
-        return this.http;
+        return Metric.HTTP.containedIn(requestedMetrics);
     }
 
     /**
      * Should the node HTTP be returned.
      */
     public NodesStatsRequest http(boolean http) {
-        this.http = http;
+        addOrRemoveMetric(http, Metric.HTTP.metricName());
         return this;
     }
 
     public boolean breaker() {
-        return this.breaker;
+        return Metric.BREAKER.containedIn(requestedMetrics);
     }
 
     /**
      * Should the node's circuit breaker stats be returned.
      */
     public NodesStatsRequest breaker(boolean breaker) {
-        this.breaker = breaker;
+        addOrRemoveMetric(breaker, Metric.BREAKER.metricName());
         return this;
     }
 
     public boolean script() {
-        return script;
+        return Metric.SCRIPT.containedIn(requestedMetrics);
     }
 
     public NodesStatsRequest script(boolean script) {
-        this.script = script;
+        addOrRemoveMetric(script, Metric.SCRIPT.metricName());
         return this;
     }
 
 
     public boolean discovery() {
-        return this.discovery;
+        return Metric.DISCOVERY.containedIn(requestedMetrics);
     }
 
     /**
      * Should the node's discovery stats be returned.
      */
     public NodesStatsRequest discovery(boolean discovery) {
-        this.discovery = discovery;
+        addOrRemoveMetric(discovery, Metric.DISCOVERY.metricName());
         return this;
     }
 
     public boolean ingest() {
-        return ingest;
+        return Metric.INGEST.containedIn(requestedMetrics);
     }
 
     /**
      * Should ingest statistics be returned.
      */
     public NodesStatsRequest ingest(boolean ingest) {
-        this.ingest = ingest;
+        addOrRemoveMetric(ingest, Metric.INGEST.metricName());
         return this;
     }
 
     public boolean adaptiveSelection() {
-        return adaptiveSelection;
+        return Metric.ADAPTIVE_SELECTION.containedIn(requestedMetrics);
     }
 
     /**
      * Should adaptiveSelection statistics be returned.
      */
     public NodesStatsRequest adaptiveSelection(boolean adaptiveSelection) {
-        this.adaptiveSelection = adaptiveSelection;
+        addOrRemoveMetric(adaptiveSelection, Metric.ADAPTIVE_SELECTION.metricName());
         return this;
     }
 
+    /**
+     * Helper method for adding and removing metrics.
+     * @param includeMetric Whether or not to include a metric.
+     * @param metricName Name of the metric to include or remove.
+     */
+    private void addOrRemoveMetric(boolean includeMetric, String metricName) {
+        if (includeMetric) {
+            requestedMetrics.add(metricName);
+        } else {
+            requestedMetrics.remove(metricName);
+        }
+    }
+
     @Override
     public void writeTo(StreamOutput out) throws IOException {
         super.writeTo(out);
         indices.writeTo(out);
-        out.writeBoolean(os);
+        if (out.getVersion().before(Version.V_7_7_0)) {
-        out.writeBoolean(process);
+            out.writeBoolean(Metric.OS.containedIn(requestedMetrics));
-        out.writeBoolean(jvm);
+            out.writeBoolean(Metric.PROCESS.containedIn(requestedMetrics));
-        out.writeBoolean(threadPool);
+            out.writeBoolean(Metric.JVM.containedIn(requestedMetrics));
-        out.writeBoolean(fs);
+            out.writeBoolean(Metric.THREAD_POOL.containedIn(requestedMetrics));
-        out.writeBoolean(transport);
+            out.writeBoolean(Metric.FS.containedIn(requestedMetrics));
-        out.writeBoolean(http);
+            out.writeBoolean(Metric.TRANSPORT.containedIn(requestedMetrics));
-        out.writeBoolean(breaker);
+            out.writeBoolean(Metric.HTTP.containedIn(requestedMetrics));
-        out.writeBoolean(script);
+            out.writeBoolean(Metric.BREAKER.containedIn(requestedMetrics));
-        out.writeBoolean(discovery);
+            out.writeBoolean(Metric.SCRIPT.containedIn(requestedMetrics));
-        out.writeBoolean(ingest);
+            out.writeBoolean(Metric.DISCOVERY.containedIn(requestedMetrics));
-        if (out.getVersion().onOrAfter(Version.V_6_1_0)) {
+            out.writeBoolean(Metric.INGEST.containedIn(requestedMetrics));
-            out.writeBoolean(adaptiveSelection);
+            if (out.getVersion().onOrAfter(Version.V_6_1_0)) {
+                out.writeBoolean(Metric.ADAPTIVE_SELECTION.containedIn(requestedMetrics));
+            }
+        } else {
+            out.writeStringArray(requestedMetrics.toArray(new String[0]));
+        }
+    }
+
+    /**
+     * An enumeration of the "core" sections of metrics that may be requested
+     * from the nodes stats endpoint. Eventually this list will be pluggable.
+     */
+    private enum Metric {
+        OS("os"),
+        PROCESS("process"),
+        JVM("jvm"),
+        THREAD_POOL("threadPool"),
+        FS("fs"),
+        TRANSPORT("transport"),
+        HTTP("http"),
+        BREAKER("breaker"),
+        SCRIPT("script"),
+        DISCOVERY("discovery"),
+        INGEST("ingest"),
+        ADAPTIVE_SELECTION("adaptiveSelection");
+
+        private String metricName;
+
+        Metric(String name) {
+            this.metricName = name;
+        }
+
+        String metricName() {
+            return this.metricName;
+        }
+
+        boolean containedIn(Set<String> metricNames) {
+            return metricNames.contains(this.metricName());
+        }
+
+        static Set<String> allMetrics() {
+            return Arrays.stream(values()).map(Metric::metricName).collect(Collectors.toSet());
         }
     }
 }

server/src/test/java/org/elasticsearch/action/admin/cluster/node/stats/NodesStatsRequestTests.java

@@ -0,0 +1,149 @@
+/*
+ * Licensed to Elasticsearch under one or more contributor
+ * license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright
+ * ownership. Elasticsearch licenses this file to you under
+ * the Apache License, Version 2.0 (the "License"); you may
+ * not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *    http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.elasticsearch.action.admin.cluster.node.stats;
+
+import org.elasticsearch.action.admin.indices.stats.CommonStatsFlags;
+import org.elasticsearch.common.io.stream.BytesStreamOutput;
+import org.elasticsearch.common.io.stream.StreamInput;
+import org.elasticsearch.test.ESTestCase;
+
+import static org.hamcrest.Matchers.equalTo;
+
+public class NodesStatsRequestTests extends ESTestCase {
+
+    /**
+     * Make sure that we can set, serialize, and deserialize arbitrary sets
+     * of metrics.
+     *
+     * TODO: Use a looping construct rather than direct API calls
+     */
+    public void testMetricsSetters() throws Exception {
+        NodesStatsRequest request = new NodesStatsRequest(randomAlphaOfLength(8));
+        request.indices(randomFrom(CommonStatsFlags.ALL));
+        request.os(randomBoolean());
+        request.process(randomBoolean());
+        request.jvm(randomBoolean());
+        request.threadPool(randomBoolean());
+        request.fs(randomBoolean());
+        request.transport(randomBoolean());
+        request.http(randomBoolean());
+        request.breaker(randomBoolean());
+        request.script(randomBoolean());
+        request.discovery(randomBoolean());
+        request.ingest(randomBoolean());
+        request.adaptiveSelection(randomBoolean());
+        NodesStatsRequest deserializedRequest = roundTripRequest(request);
+        assertRequestsEqual(request, deserializedRequest);
+    }
+
+    /**
+     * Test that a newly constructed NodesStatsRequestObject requests all of the
+     * possible metrics defined in {@link NodesStatsRequest}.
+     */
+    public void testNodesStatsRequestDefaults() {
+        NodesStatsRequest defaultNodesStatsRequest = new NodesStatsRequest(randomAlphaOfLength(8));
+        NodesStatsRequest constructedNodesStatsRequest = new NodesStatsRequest(randomAlphaOfLength(8));
+        constructedNodesStatsRequest.clear();
+        constructedNodesStatsRequest.indices(CommonStatsFlags.ALL);
+
+        assertRequestsEqual(defaultNodesStatsRequest, constructedNodesStatsRequest);
+    }
+
+    /**
+     * Test that the {@link NodesStatsRequest#all()} method sets all of the
+     * metrics to {@code true}.
+     *
+     * TODO: Use a looping construct rather than direct API calls
+     */
+    public void testNodesInfoRequestAll() throws Exception {
+        NodesStatsRequest request = new NodesStatsRequest("node");
+        request.all();
+
+        assertThat(request.indices().getFlags(), equalTo(CommonStatsFlags.ALL.getFlags()));
+        assertTrue(request.os());
+        assertTrue(request.process());
+        assertTrue(request.jvm());
+        assertTrue(request.threadPool());
+        assertTrue(request.fs());
+        assertTrue(request.transport());
+        assertTrue(request.http());
+        assertTrue(request.breaker());
+        assertTrue(request.script());
+        assertTrue(request.discovery());
+        assertTrue(request.ingest());
+        assertTrue(request.adaptiveSelection());
+    }
+
+    /**
+     * Test that the {@link NodesStatsRequest#clear()} method sets all of the
+     * metrics to {@code false}.
+     *
+     * TODO: Use a looping construct rather than direct API calls
+     */
+    public void testNodesInfoRequestClear() throws Exception {
+        NodesStatsRequest request = new NodesStatsRequest("node");
+        request.clear();
+
+        assertThat(request.indices().getFlags(), equalTo(CommonStatsFlags.NONE.getFlags()));
+        assertFalse(request.os());
+        assertFalse(request.process());
+        assertFalse(request.jvm());
+        assertFalse(request.threadPool());
+        assertFalse(request.fs());
+        assertFalse(request.transport());
+        assertFalse(request.http());
+        assertFalse(request.breaker());
+        assertFalse(request.script());
+        assertFalse(request.discovery());
+        assertFalse(request.ingest());
+        assertFalse(request.adaptiveSelection());
+    }
+
+    /**
+     * Serialize and deserialize a request.
+     * @param request A request to serialize.
+     * @return The deserialized, "round-tripped" request.
+     */
+    private static NodesStatsRequest roundTripRequest(NodesStatsRequest request) throws Exception {
+        try (BytesStreamOutput out = new BytesStreamOutput()) {
+            request.writeTo(out);
+            try (StreamInput in = out.bytes().streamInput()) {
+                return new NodesStatsRequest(in);
+            }
+        }
+    }
+
+    private static void assertRequestsEqual(NodesStatsRequest request1, NodesStatsRequest request2) {
+        // TODO: Use a looping construct rather than direct API calls
+        assertThat(request1.indices().getFlags(), equalTo(request2.indices().getFlags()));
+        assertThat(request1.os(), equalTo(request2.os()));
+        assertThat(request1.process(), equalTo(request2.process()));
+        assertThat(request1.jvm(), equalTo(request2.jvm()));
+        assertThat(request1.threadPool(), equalTo(request2.threadPool()));
+        assertThat(request1.fs(), equalTo(request2.fs()));
+        assertThat(request1.transport(), equalTo(request2.transport()));
+        assertThat(request1.http(), equalTo(request2.http()));
+        assertThat(request1.breaker(), equalTo(request2.breaker()));
+        assertThat(request1.script(), equalTo(request2.script()));
+        assertThat(request1.discovery(), equalTo(request2.discovery()));
+        assertThat(request1.ingest(), equalTo(request2.ingest()));
+        assertThat(request1.adaptiveSelection(), equalTo(request2.adaptiveSelection()));
+    }
+}