Add builder to construct InstanceSpec (#1275)

It is hard for us to know what does
`new InstanceSpec(null, -1, -1, -1, true, -1, -1, -1)` means now.

Closes #1222.

Author: kezhuw

curator-test/src/main/java/org/apache/curator/test/InstanceSpec.java

@@ -70,8 +70,15 @@ public static void reset() {
         nextServerId.set(1);
     }
 
+    /**
+     * Create a builder to construct {@link InstanceSpec}.
+     */
+    public static InstanceSpecBuilder builder() {
+        return new InstanceSpecBuilder();
+    }
+
     public static InstanceSpec newInstanceSpec() {
-        return new InstanceSpec(null, -1, -1, -1, true, -1, -1, -1);
+        return builder().build();
     }
 
     public static int getRandomPort() {
@@ -94,13 +101,16 @@ public static int getRandomPort() {
     }
 
     /**
+     * @deprecated Use {@link #builder()} instead.
+     *
      * @param dataDirectory              where to store data/logs/etc.
      * @param port                       the port to listen on - each server in the ensemble must use a unique port
      * @param electionPort               the electionPort to listen on - each server in the ensemble must use a unique electionPort
      * @param quorumPort                 the quorumPort to listen on - each server in the ensemble must use a unique quorumPort
      * @param deleteDataDirectoryOnClose if true, the data directory will be deleted when {@link TestingCluster#close()} is called
      * @param serverId                   the server ID for the instance
      */
+    @Deprecated
     public InstanceSpec(
             File dataDirectory,
             int port,
@@ -112,6 +122,8 @@ public InstanceSpec(
     }
 
     /**
+     * @deprecated Use {@link #builder()} instead.
+     *
      * @param dataDirectory              where to store data/logs/etc.
      * @param port                       the port to listen on - each server in the ensemble must use a unique port
      * @param electionPort               the electionPort to listen on - each server in the ensemble must use a unique electionPort
@@ -121,6 +133,7 @@ public InstanceSpec(
      * @param tickTime                   tickTime. Set -1 to used fault server configuration
      * @param maxClientCnxns             max number of client connections from the same IP. Set -1 to use default server configuration
      */
+    @Deprecated
     public InstanceSpec(
             File dataDirectory,
             int port,
@@ -144,6 +157,8 @@ public InstanceSpec(
     }
 
     /**
+     * @deprecated Use {@link #builder()} instead.
+     *
      * @param dataDirectory              where to store data/logs/etc.
      * @param port                       the port to listen on - each server in the ensemble must use a unique port
      * @param electionPort               the electionPort to listen on - each server in the ensemble must use a unique electionPort
@@ -154,6 +169,7 @@ public InstanceSpec(
      * @param maxClientCnxns             max number of client connections from the same IP. Set -1 to use default server configuration
      * @param customProperties           other properties to be passed to the server
      */
+    @Deprecated
     public InstanceSpec(
             File dataDirectory,
             int port,
@@ -178,6 +194,8 @@ public InstanceSpec(
     }
 
     /**
+     * @deprecated Use {@link #builder()} instead.
+     *
      * @param dataDirectory              where to store data/logs/etc.
      * @param port                       the port to listen on - each server in the ensemble must use a unique port
      * @param electionPort               the electionPort to listen on - each server in the ensemble must use a unique electionPort
@@ -189,6 +207,7 @@ public InstanceSpec(
      * @param customProperties           other properties to be passed to the server
      * @param hostname                   Hostname or IP if the cluster is intending to be bounded into external interfaces
      */
+    @Deprecated
     public InstanceSpec(
             File dataDirectory,
             int port,
@@ -200,6 +219,32 @@ public InstanceSpec(
             int maxClientCnxns,
             Map<String, Object> customProperties,
             String hostname) {
+        this(
+                dataDirectory,
+                port,
+                electionPort,
+                quorumPort,
+                deleteDataDirectoryOnClose,
+                serverId,
+                tickTime,
+                maxClientCnxns,
+                customProperties != null ? enforceStringMap(customProperties) : null,
+                hostname,
+                false);
+    }
+
+    InstanceSpec(
+            File dataDirectory,
+            int port,
+            int electionPort,
+            int quorumPort,
+            boolean deleteDataDirectoryOnClose,
+            int serverId,
+            int tickTime,
+            int maxClientCnxns,
+            Map<String, Object> customProperties,
+            String hostname,
+            boolean ignored) {
         this.dataDirectory = (dataDirectory != null) ? dataDirectory : DirectoryUtils.createTempDirectory();
         this.port = (port >= 0) ? port : getRandomPort();
         this.electionPort = (electionPort >= 0) ? electionPort : getRandomPort();
@@ -208,9 +253,8 @@ public InstanceSpec(
         this.serverId = (serverId >= 0) ? serverId : nextServerId.getAndIncrement();
         this.tickTime = (tickTime > 0 ? tickTime : -1); // -1 to set default value
         this.maxClientCnxns = (maxClientCnxns >= 0 ? maxClientCnxns : -1); // -1 to set default value
-        this.customProperties = customProperties != null
-                ? Collections.unmodifiableMap(enforceStringMap(customProperties))
-                : Collections.emptyMap();
+        this.customProperties =
+                customProperties != null ? Collections.unmodifiableMap(customProperties) : Collections.emptyMap();
         this.hostname = hostname == null ? localhost : hostname;
     }
 

curator-test/src/main/java/org/apache/curator/test/InstanceSpecBuilder.java

@@ -0,0 +1,111 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF 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.apache.curator.test;
+
+import java.io.File;
+import java.util.Map;
+
+/**
+ * Builder to construct {@link InstanceSpec}.
+ */
+public class InstanceSpecBuilder {
+    private File dataDirectory;
+    private boolean deleteDataDirectoryOnClose = true;
+    private int port = -1;
+    private int electionPort = -1;
+    private int quorumPort = -1;
+    private int serverId = -1;
+    private int tickTime = -1;
+    private int maxClientCnxns = -1;
+    private Map<String, String> customProperties = null;
+    private String hostname = null;
+
+    /**
+     * Data directory to store data of this server instance. It will be deleted upon sever close.
+     *
+     * @see #withDataDirectory(File, boolean) also
+     */
+    public InstanceSpecBuilder withDataDirectory(File dataDirectory) {
+        this.dataDirectory = dataDirectory;
+        return this;
+    }
+
+    public InstanceSpecBuilder withDataDirectory(File dataDirectory, boolean deleteDataDirectoryOnClose) {
+        this.dataDirectory = dataDirectory;
+        this.deleteDataDirectoryOnClose = deleteDataDirectoryOnClose;
+        return this;
+    }
+
+    public InstanceSpecBuilder withPort(int port) {
+        this.port = port;
+        return this;
+    }
+
+    public InstanceSpecBuilder withElectionPort(int electionPort) {
+        this.electionPort = electionPort;
+        return this;
+    }
+
+    public InstanceSpecBuilder withQuorumPort(int quorumPort) {
+        this.quorumPort = quorumPort;
+        return this;
+    }
+
+    public InstanceSpecBuilder withServerId(int serverId) {
+        this.serverId = serverId;
+        return this;
+    }
+
+    public InstanceSpecBuilder withTickTime(int tickTime) {
+        this.tickTime = tickTime;
+        return this;
+    }
+
+    public InstanceSpecBuilder withMaxClientCnxns(int maxClientCnxns) {
+        this.maxClientCnxns = maxClientCnxns;
+        return this;
+    }
+
+    public InstanceSpecBuilder withCustomProperties(Map<String, String> customProperties) {
+        this.customProperties = customProperties;
+        return this;
+    }
+
+    public InstanceSpecBuilder withHostname(String hostname) {
+        this.hostname = hostname;
+        return this;
+    }
+
+    @SuppressWarnings("unchecked")
+    public InstanceSpec build() {
+        return new InstanceSpec(
+                dataDirectory,
+                port,
+                electionPort,
+                quorumPort,
+                deleteDataDirectoryOnClose,
+                serverId,
+                tickTime,
+                maxClientCnxns,
+                (Map) customProperties,
+                hostname,
+                true);
+    }
+}

curator-test/src/main/java/org/apache/curator/test/TestingServer.java

@@ -94,7 +94,12 @@ public TestingServer(int port, File tempDirectory) throws Exception {
      * @throws Exception errors
      */
     public TestingServer(int port, File tempDirectory, boolean start) throws Exception {
-        this(new InstanceSpec(tempDirectory, Math.max(0, port), -1, -1, true, -1), start);
+        this(
+                InstanceSpec.builder()
+                        .withPort(Math.max(0, port))
+                        .withDataDirectory(tempDirectory)
+                        .build(),
+                start);
     }
 
     /**

curator-test/src/test/java/org/apache/curator/test/TestQuorumConfigBuilder.java

@@ -31,11 +31,14 @@ public class TestQuorumConfigBuilder {
 
     @Test
     public void testCustomProperties() throws Exception {
-        Map<String, Object> customProperties = new HashMap<String, Object>();
+        Map<String, String> customProperties = new HashMap();
         customProperties.put("authProvider.1", "org.apache.zookeeper.server.auth.SASLAuthenticationProvider");
         customProperties.put("kerberos.removeHostFromPrincipal", "true");
         customProperties.put("kerberos.removeRealmFromPrincipal", "true");
-        InstanceSpec spec = new InstanceSpec(null, -1, -1, -1, true, 1, -1, -1, customProperties);
+        InstanceSpec spec = InstanceSpec.builder()
+                .withServerId(1)
+                .withCustomProperties(customProperties)
+                .build();
         TestingServer server = new TestingServer(spec, true);
         try {
             assertEquals(

curator-test/src/test/java/org/apache/curator/test/TestTestingServer.java

@@ -41,7 +41,11 @@ public void setCustomTickTimeTest() throws Exception {
         } else {
             customTickMs = 100;
         }
-        final InstanceSpec spec = new InstanceSpec(zkTmpDir, -1, -1, -1, true, -1, customTickMs, -1);
+        final InstanceSpec spec = InstanceSpec.builder()
+                .withDataDirectory(zkTmpDir)
+                .withTickTime(customTickMs)
+                .build();
+        ;
         final int zkTickTime;
         try (TestingServer testingServer = new TestingServer(spec, true)) {
             TestingZooKeeperMain main = (TestingZooKeeperMain)