From c03ad5abc3977f67cb85a0bf433cd12a456bc4e9 Mon Sep 17 00:00:00 2001
From: Shams Asari <shams.asari@r3.com>
Date: Mon, 3 Dec 2018 17:25:56 +0000
Subject: [PATCH] CORDA-2254: Replaced JmxPolicy.httpPort with the original
 PortAllocation (#4345)

The httpPort parameter is fundamentally wrong as it gives the same monitoring port to each node!

Added JmxPolicy.defaultEnabled() to workaround the (incorrect) default false value of startJmxHttpServer.
---
 .ci/api-current.txt                           |  4 +-
 .../net/corda/testing/driver/DriverTests.kt   |  4 +-
 .../kotlin/net/corda/testing/driver/Driver.kt | 30 ++++++++++-
 .../net/corda/testing/driver/JmxPolicy.kt     | 50 -------------------
 .../testing/node/internal/DriverDSLImpl.kt    |  6 +--
 .../net/corda/explorer/ExplorerSimulation.kt  |  2 +-
 6 files changed, 36 insertions(+), 60 deletions(-)
 delete mode 100644 testing/node-driver/src/main/kotlin/net/corda/testing/driver/JmxPolicy.kt

diff --git a/.ci/api-current.txt b/.ci/api-current.txt
index f76e1a94bd..f647ff6b38 100644
--- a/.ci/api-current.txt
+++ b/.ci/api-current.txt
@@ -5976,12 +5976,12 @@ public final class net.corda.testing.driver.JmxPolicy extends java.lang.Object
   public <init>()
   public <init>(boolean, net.corda.testing.driver.PortAllocation)
   public final boolean component1()
-  @Nullable
+  @NotNull
   public final net.corda.testing.driver.PortAllocation component2()
   @NotNull
   public final net.corda.testing.driver.JmxPolicy copy(boolean, net.corda.testing.driver.PortAllocation)
   public boolean equals(Object)
-  @Nullable
+  @NotNull
   public final net.corda.testing.driver.PortAllocation getJmxHttpServerPortAllocation()
   public final boolean getStartJmxHttpServer()
   public int hashCode()
diff --git a/testing/node-driver/src/integration-test/kotlin/net/corda/testing/driver/DriverTests.kt b/testing/node-driver/src/integration-test/kotlin/net/corda/testing/driver/DriverTests.kt
index c57b299134..889cecf5c3 100644
--- a/testing/node-driver/src/integration-test/kotlin/net/corda/testing/driver/DriverTests.kt
+++ b/testing/node-driver/src/integration-test/kotlin/net/corda/testing/driver/DriverTests.kt
@@ -12,7 +12,6 @@ import net.corda.testing.common.internal.ProjectStructure.projectRootDir
 import net.corda.testing.core.BOB_NAME
 import net.corda.testing.core.DUMMY_BANK_A_NAME
 import net.corda.testing.core.DUMMY_BANK_B_NAME
-import net.corda.testing.driver.internal.incrementalPortAllocation
 import net.corda.testing.http.HttpApi
 import net.corda.testing.node.internal.addressMustBeBound
 import net.corda.testing.node.internal.addressMustNotBeBound
@@ -101,8 +100,7 @@ class DriverTests {
 
     @Test
     fun `monitoring mode enables jolokia exporting of JMX metrics via HTTP JSON`() {
-        val portAllocation = incrementalPortAllocation(7006)
-        driver(DriverParameters(jmxPolicy = JmxPolicy(portAllocation.nextPort()), startNodesInProcess = false, notarySpecs = emptyList())) {
+        driver(DriverParameters(jmxPolicy = JmxPolicy.defaultEnabled(), startNodesInProcess = false, notarySpecs = emptyList())) {
             val node = startNode(providedName = DUMMY_REGULATOR_NAME).getOrThrow()
             // request access to some JMX metrics via Jolokia HTTP/JSON
             val api = HttpApi.fromHostAndPort(node.jmxAddress!!, "/jolokia/")
diff --git a/testing/node-driver/src/main/kotlin/net/corda/testing/driver/Driver.kt b/testing/node-driver/src/main/kotlin/net/corda/testing/driver/Driver.kt
index 09cbfc2228..15b5420298 100644
--- a/testing/node-driver/src/main/kotlin/net/corda/testing/driver/Driver.kt
+++ b/testing/node-driver/src/main/kotlin/net/corda/testing/driver/Driver.kt
@@ -121,6 +121,34 @@ abstract class PortAllocation {
     }
 }
 
+/**
+ * A class containing configuration information for Jolokia JMX, to be used when creating a node via the [driver].
+ *
+ * @property startJmxHttpServer Indicates whether the spawned nodes should start with a Jolokia JMX agent to enable remote
+ * JMX monitoring using HTTP/JSON.
+ * @property jmxHttpServerPortAllocation The port allocation strategy to use for remote Jolokia/JMX monitoring over HTTP.
+ * Defaults to incremental from port 7005. Use [NodeHandle.jmxAddress] to get the assigned address.
+ */
+@Suppress("DEPRECATION")
+data class JmxPolicy
+@Deprecated("Use the constructor that just takes in the jmxHttpServerPortAllocation or use JmxPolicy.defaultEnabled()")
+constructor(
+        val startJmxHttpServer: Boolean = false,
+        val jmxHttpServerPortAllocation: PortAllocation = incrementalPortAllocation(7005)
+) {
+    @Deprecated("The default constructor does not turn on monitoring. Simply leave the jmxPolicy parameter unspecified if you wish to not have monitoring turned on.")
+    constructor() : this(false)
+
+    /** Create a [JmxPolicy] that turns on monitoring using the given [PortAllocation]. */
+    constructor(jmxHttpServerPortAllocation: PortAllocation) : this(true, jmxHttpServerPortAllocation)
+
+    companion object {
+        /** Returns a default [JmxPolicy] that turns on monitoring. */
+        @JvmStatic
+        fun defaultEnabled(): JmxPolicy = JmxPolicy(true)
+    }
+}
+
 /**
  * [driver] allows one to start up nodes like this:
  *   driver {
@@ -136,7 +164,7 @@ abstract class PortAllocation {
  *
  * @param defaultParameters The default parameters for the driver. Allows the driver to be configured in builder style
  *   when called from Java code.
- * @property dsl The dsl itself.
+ * @param dsl The dsl itself.
  * @return The value returned in the [dsl] closure.
  */
 fun <A> driver(defaultParameters: DriverParameters = DriverParameters(), dsl: DriverDSL.() -> A): A {
diff --git a/testing/node-driver/src/main/kotlin/net/corda/testing/driver/JmxPolicy.kt b/testing/node-driver/src/main/kotlin/net/corda/testing/driver/JmxPolicy.kt
deleted file mode 100644
index 26882208be..0000000000
--- a/testing/node-driver/src/main/kotlin/net/corda/testing/driver/JmxPolicy.kt
+++ /dev/null
@@ -1,50 +0,0 @@
-package net.corda.testing.driver
-
-/**
- * A class containing configuration information for Jolokia JMX, to be used when creating a node via the [driver].
- *
- * @property httpPort The port to use for remote Jolokia/JMX monitoring over HTTP. Defaults to 7006.
- */
-@Suppress("DEPRECATION")
-class JmxPolicy private constructor(
-        @Deprecated("This is no longer needed to turn on monitoring.")
-        val startJmxHttpServer: Boolean,
-        @Deprecated("This has been replaced by httpPort which makes it clear to the calling code which port will be used.")
-        val jmxHttpServerPortAllocation: PortAllocation?,
-        val httpPort: Int
-) {
-    /** Create a JmxPolicy that turns on monitoring on the given [httpPort]. */
-    constructor(httpPort: Int) : this(true, null, httpPort)
-
-    override fun equals(other: Any?): Boolean {
-        if (this === other) return true
-        if (other !is JmxPolicy) return false
-        return this.httpPort == other.httpPort
-    }
-
-    override fun hashCode(): Int {
-        var result = httpPort.hashCode()
-        result = 31 * result + httpPort
-        return result
-    }
-
-    override fun toString(): String = "JmxPolicy(httpPort=$httpPort)"
-
-    // The below cannot be removed as they're already part of the public API, so it's deprecated instead
-
-    @Deprecated("The default constructor does not turn on monitoring. Simply leave the jmxPolicy parameter unspecified.")
-    constructor() : this(false, null, 7006)
-    @Deprecated("Use constructor that takes in the httpPort")
-    constructor(startJmxHttpServer: Boolean = false, jmxHttpServerPortAllocation: PortAllocation? = null) : this(startJmxHttpServer, jmxHttpServerPortAllocation, 7006)
-
-    @Deprecated("startJmxHttpServer is deprecated as it's no longer needed to turn on monitoring.")
-    operator fun component1(): Boolean = startJmxHttpServer
-    @Deprecated("jmxHttpServerPortAllocation is deprecated and no longer does anything. Use httpPort instead.")
-    operator fun component2(): PortAllocation? = jmxHttpServerPortAllocation
-
-    @Deprecated("startJmxHttpServer and jmxHttpServerPortAllocation are both deprecated.")
-    fun copy(startJmxHttpServer: Boolean = this.startJmxHttpServer,
-             jmxHttpServerPortAllocation: PortAllocation? = this.jmxHttpServerPortAllocation): JmxPolicy {
-        return JmxPolicy(startJmxHttpServer, jmxHttpServerPortAllocation)
-    }
-}
diff --git a/testing/node-driver/src/main/kotlin/net/corda/testing/node/internal/DriverDSLImpl.kt b/testing/node-driver/src/main/kotlin/net/corda/testing/node/internal/DriverDSLImpl.kt
index 92e6e370d3..2decaddae4 100644
--- a/testing/node-driver/src/main/kotlin/net/corda/testing/node/internal/DriverDSLImpl.kt
+++ b/testing/node-driver/src/main/kotlin/net/corda/testing/node/internal/DriverDSLImpl.kt
@@ -221,9 +221,9 @@ class DriverDSLImpl(
                         "networkServices.networkMapURL" to compatibilityZone.networkMapURL().toString())
         }
 
-        @Suppress("DEPRECATION")
-        val jmxConfig = if (jmxPolicy.startJmxHttpServer) {
-            mapOf(NodeConfiguration::jmxMonitoringHttpPort.name to jmxPolicy.httpPort)
+        val jmxPort = if (jmxPolicy.startJmxHttpServer) jmxPolicy.jmxHttpServerPortAllocation.nextPort() else null
+        val jmxConfig = if (jmxPort != null) {
+            mapOf(NodeConfiguration::jmxMonitoringHttpPort.name to jmxPort)
         } else {
             emptyMap()
         }
diff --git a/tools/explorer/src/main/kotlin/net/corda/explorer/ExplorerSimulation.kt b/tools/explorer/src/main/kotlin/net/corda/explorer/ExplorerSimulation.kt
index 30fdd91d9f..d24b3656ad 100644
--- a/tools/explorer/src/main/kotlin/net/corda/explorer/ExplorerSimulation.kt
+++ b/tools/explorer/src/main/kotlin/net/corda/explorer/ExplorerSimulation.kt
@@ -73,7 +73,7 @@ class ExplorerSimulation(private val options: OptionSet) {
                 portAllocation = portAllocation,
                 cordappsForAllNodes = listOf(FINANCE_CORDAPP),
                 waitForAllNodesToFinish = true,
-                jmxPolicy = JmxPolicy(7006)
+                jmxPolicy = JmxPolicy.defaultEnabled()
         )) {
             // TODO : Supported flow should be exposed somehow from the node instead of set of ServiceInfo.
             val alice = startNode(providedName = ALICE_NAME, rpcUsers = listOf(user))