Fix #2727, combine -jettyThreads and -jettyMaxThreads (#2735)

- Revert BaseRemoteProxy configuration load behavior to Sel 2.x logic (seed from registry, then overlap with the remote request) - Combine -jettyThreads (hidden, used by standalone and node) and -jettyMaxThreads (not hidden, used by hub) into -jettyMaxThreads which is NOT hidden and is used by standalone, node, and hub Additional changes; - Fix for 'id' which should have a default value applied (according to its usage doc) - new GridHubConfiguration() - the role should always be "hub" - new GridNodeConfiguration() - the role should always be "node" - Configuration merge() behavior update/change; - don't merge null 'other' values (pre-existing) - don't merge empty collections or maps (new) - Add tests
SeleniumHQ · Sep 8, 2016 · 031456c · 031456c
1 parent 270e0c8
commit 031456c
Show file tree

Hide file tree

Showing 15 changed files with 667 additions and 172 deletions.
diff --git a/java/server/src/org/openqa/grid/common/RegistrationRequest.java b/java/server/src/org/openqa/grid/common/RegistrationRequest.java
@@ -140,8 +140,11 @@ public static RegistrationRequest getNewInstance(String json) throws JsonSyntaxE
     GridNodeConfiguration configuration = GridNodeConfiguration.loadFromJSON(config);
     request.setConfiguration(configuration);
 
+    // make sure 'id' has a value
     if (o.has("id")) {
       request.configuration.id = o.get("id").getAsString();
+    } else {
+      request.configuration.id = request.configuration.getRemoteHost();
     }
 
     JsonArray capabilities = o.get("capabilities").getAsJsonArray();

diff --git a/java/server/src/org/openqa/grid/internal/BaseRemoteProxy.java b/java/server/src/org/openqa/grid/internal/BaseRemoteProxy.java
@@ -104,14 +104,14 @@ public BaseRemoteProxy(RegistrationRequest request, Registry registry) {
     this.request = request;
     this.registry = registry;
     this.config = new GridNodeConfiguration();
-    this.config.merge(request.getConfiguration());
-    // the registry is the 'hub' configuration, which takes precedence.
-    // merging last overrides any other values.
+    // the registry is the 'hub' configuration, which is used as a seed.
     this.config.merge(registry.getConfiguration());
+    // the proxy values must override any that the hub specify where an overlap occurs.
+    // merging last causes the values to be overridden.
+    this.config.merge(request.getConfiguration());
+    // host and port are merge() protected values -- overrule this behavior
     this.config.host = request.getConfiguration().host;
     this.config.port = request.getConfiguration().port;
-    // custom configurations from the remote need to 'override' the hub
-    this.config.custom.putAll(request.getConfiguration().custom);
 
     String url = config.getRemoteHost();
     String id = config.id;

diff --git a/java/server/src/org/openqa/grid/internal/utils/configuration/GridConfiguration.java b/java/server/src/org/openqa/grid/internal/utils/configuration/GridConfiguration.java
@@ -73,17 +73,20 @@ public class GridConfiguration extends StandaloneConfiguration {
   public void merge(GridConfiguration other) {
     super.merge(other);
     // don't merge 'host'
-    if (other.cleanUpCycle != null) {
+    if (isMergeAble(other.cleanUpCycle, cleanUpCycle)) {
       cleanUpCycle = other.cleanUpCycle;
     }
-    custom.putAll(other.custom);
-    if (other.maxSession != 1) {
+    if (isMergeAble(other.custom, custom)) {
+      custom.putAll(other.custom);
+    }
+    if (isMergeAble(other.maxSession, maxSession) &&
+        other.maxSession.intValue() > 0) {
       maxSession = other.maxSession;
     }
-    if (other.servlets != null) {
+    if (isMergeAble(other.servlets, servlets)) {
       servlets = other.servlets;
     }
-    if (other.withoutServlets != null) {
+    if (isMergeAble(other.withoutServlets, withoutServlets)) {
       withoutServlets = other.withoutServlets;
     }
   }

diff --git a/java/server/src/org/openqa/grid/internal/utils/configuration/GridHubConfiguration.java b/java/server/src/org/openqa/grid/internal/utils/configuration/GridHubConfiguration.java
@@ -44,12 +44,6 @@ public class GridHubConfiguration extends GridConfiguration {
   )
   public String hubConfig;
 
-  @Parameter(
-    names = "-jettyMaxThreads",
-    description = "<Integer> : max number of thread for Jetty. Default is 255"
-  )
-  public Integer jettyMaxThreads;
-
   @Parameter(
     names = {"-matcher", "-capabilityMatcher"},
     description = "<String> class name : a class implementing the CapabilityMatcher interface. Specifies the logic the hub will follow to define whether a request can be assigned to a node. For example, if you want to have the matching process use regular expressions instead of exact match when specifying browser version. ALL nodes of a grid ecosystem would then use the same capabilityMatcher, as defined here. Default is org.openqa.grid.internal.utils.DefaultCapabilityMatcher",
@@ -82,6 +76,7 @@ public class GridHubConfiguration extends GridConfiguration {
    * Init with built-in defaults
    */
   public GridHubConfiguration() {
+    role = "hub";
     if (DEFAULT_CONFIG != null) {
       merge(DEFAULT_CONFIG);
     }
@@ -139,17 +134,17 @@ public void merge(GridNodeConfiguration other) {
 
   public void merge(GridHubConfiguration other) {
     super.merge(other);
-    if (other.jettyMaxThreads != null) {
-      jettyMaxThreads = other.jettyMaxThreads;
+
+    if (isMergeAble(other.capabilityMatcher, capabilityMatcher)) {
+      capabilityMatcher = other.capabilityMatcher;
     }
-    capabilityMatcher = other.capabilityMatcher;
-    if (other.newSessionWaitTimeout != null) {
+    if (isMergeAble(other.newSessionWaitTimeout, newSessionWaitTimeout)) {
       newSessionWaitTimeout = other.newSessionWaitTimeout;
     }
-    if (other.prioritizer != null) {
+    if (isMergeAble(other.prioritizer, prioritizer)) {
       prioritizer = other.prioritizer;
     }
-    if (other.throwOnCapabilityNotPresent != throwOnCapabilityNotPresent) {
+    if (isMergeAble(other.throwOnCapabilityNotPresent, throwOnCapabilityNotPresent)) {
       throwOnCapabilityNotPresent = other.throwOnCapabilityNotPresent;
     }
   }
@@ -159,7 +154,6 @@ public String toString(String format) {
     StringBuilder sb = new StringBuilder();
     sb.append(super.toString(format));
     sb.append(toString(format, "hubConfig", hubConfig));
-    sb.append(toString(format, "jettyMaxThreads", jettyMaxThreads));
     sb.append(toString(format, "capabilityMatcher", capabilityMatcher.getClass().getCanonicalName()));
     sb.append(toString(format, "newSessionWaitTimeout", newSessionWaitTimeout));
     sb.append(toString(format, "prioritizer", prioritizer != null? prioritizer.getClass().getCanonicalName(): null));

diff --git a/java/server/src/org/openqa/grid/internal/utils/configuration/GridNodeConfiguration.java b/java/server/src/org/openqa/grid/internal/utils/configuration/GridNodeConfiguration.java
@@ -49,7 +49,7 @@ public class GridNodeConfiguration extends GridConfiguration {
 
   @Parameter(
     names = "-id",
-    description = "<String> : unique identifier for the node. Not required--by default, grid will use the url of the remoteHost"
+    description = "<String> : optional unique identifier for the node. Defaults to the url of the remoteHost"
   )
   public String id;
 
@@ -125,10 +125,17 @@ public class GridNodeConfiguration extends GridConfiguration {
 
   @Parameter(
     names = "-unregisterIfStillDownAfter",
-    description = "<Integer> in ms : if the node remains down for more than [unregisterIfStillDownAfter] ms, it will step attempting to re-register from the hub. Default is 6000 (1 minute)"
+    description = "<Integer> in ms : if the node remains down for more than [unregisterIfStillDownAfter] ms, it will stop attempting to re-register from the hub. Default is 6000 (1 minute)"
   )
   public Integer unregisterIfStillDownAfter;
 
+  /**
+   * Init with built-in defaults
+   */
+  public GridNodeConfiguration() {
+    role = "node";
+  }
+
   public String getHubHost() {
     if (hubHost == null) {
       if (hub == null) {
@@ -174,43 +181,43 @@ private void parseHubUrl() {
 
   public void merge(GridNodeConfiguration other) {
     super.merge(other);
-    if (other.browser != null) {
+    if (isMergeAble(other.browser, browser)) {
       browser = other.browser;
     }
-    if (other.downPollingLimit != null) {
+    if (isMergeAble(other.downPollingLimit, downPollingLimit)) {
       downPollingLimit = other.downPollingLimit;
     }
-    if (other.hub != null) {
+    if (isMergeAble(other.hub, hub)) {
       hub = other.hub;
     }
-    if (other.hubHost != null) {
+    if (isMergeAble(other.hubHost, hubHost)) {
       hubHost = other.hubHost;
     }
-    if (other.hubPort != null) {
+    if (isMergeAble(other.hubPort, hubPort)) {
       hubPort = other.hubPort;
     }
-    if (other.id != null) {
+    if (isMergeAble(other.id, id)) {
       id = other.id;
     }
-    if (other.nodePolling != null) {
+    if (isMergeAble(other.nodePolling, nodePolling)) {
       nodePolling = other.nodePolling;
     }
-    if (other.nodeStatusCheckTimeout != null) {
+    if (isMergeAble(other.nodeStatusCheckTimeout, nodeStatusCheckTimeout)) {
       nodeStatusCheckTimeout = other.nodeStatusCheckTimeout;
     }
-    if (other.proxy != null) {
+    if (isMergeAble(other.proxy, proxy)) {
       proxy = other.proxy;
     }
-    if (other.register != null) {
+    if (isMergeAble(other.register, register)) {
       register = other.register;
     }
-    if (other.registerCycle != null) {
+    if (isMergeAble(other.registerCycle, registerCycle)) {
       registerCycle = other.registerCycle;
     }
-    if (other.remoteHost != null) {
+    if (isMergeAble(other.remoteHost, remoteHost)) {
       remoteHost = other.remoteHost;
     }
-    if (other.unregisterIfStillDownAfter != null) {
+    if (isMergeAble(other.unregisterIfStillDownAfter, unregisterIfStillDownAfter)) {
       unregisterIfStillDownAfter = other.unregisterIfStillDownAfter;
     }
   }

diff --git a/java/server/src/org/openqa/grid/internal/utils/configuration/StandaloneConfiguration.java b/java/server/src/org/openqa/grid/internal/utils/configuration/StandaloneConfiguration.java
@@ -38,13 +38,15 @@ public class StandaloneConfiguration {
   @Parameter(
     names = "-browserSideLog",
     description = "DO NOT USE: Provided for compatibility with 2.0",
-    hidden = true)
+    hidden = true
+  )
   public boolean browserSideLog;
 
   @Parameter(
     names = "-captureLogsOnQuit",
     description = "DO NOT USE: Provided for compatibility with 2.0",
-    hidden = true)
+    hidden = true
+  )
   public boolean captureLogsOnQuit;
 
   @Parameter(
@@ -62,10 +64,10 @@ public class StandaloneConfiguration {
   public boolean help;
 
   @Parameter(
-    names = "-jettyThreads",
-    hidden = true
+    names = {"-jettyThreads", "-jettyMaxThreads"},
+    description = "<Integer> : max number of threads for Jetty. Default is 200"
   )
-  public Integer jettyThreads;
+  public Integer jettyMaxThreads;
 
   @Parameter(
     names = "-log",
@@ -93,40 +95,78 @@ public class StandaloneConfiguration {
 
   @Parameter(
     names = {"-timeout", "-sessionTimeout"},
-    description = "<Integer> in seconds : Specifies the timeout before the hub automatically kills a session that hasn't had any activity in the last X seconds. The test slot will then be released for another test to use. This is typically used to take care of client crashes. For grid hub/node roles, cleanUpCycle must also be set. Default is 1800 (30 minutes)"
+    description = "<Integer> in seconds : Specifies the timeout before the server automatically kills a session that hasn't had any activity in the last X seconds. The test slot will then be released for another test to use. This is typically used to take care of client crashes. For grid hub/node roles, cleanUpCycle must also be set. Default is 1800 (30 minutes)"
   )
   public Integer timeout = 1800;
 
   @Parameter(
     names = {"-avoidProxy"},
-    description = "DO NOT USE. Hack to allow selenium 3.0 server run in SauceLabs",
+    description = "DO NOT USE: Hack to allow selenium 3.0 server run in SauceLabs",
     hidden = true
   )
-  private Boolean avoidProxy;
+  private boolean avoidProxy;
 
   /**
    * copy another configuration's values into this one if they are set.
    * @param other
    */
   public void merge(StandaloneConfiguration other) {
-    if (other.browserTimeout != null) {
+    if (isMergeAble(other.browserTimeout, browserTimeout)) {
       browserTimeout = other.browserTimeout;
     }
-    if (other.jettyThreads != null) {
-      jettyThreads = other.jettyThreads;
+    if (isMergeAble(other.jettyMaxThreads, jettyMaxThreads)) {
+      jettyMaxThreads = other.jettyMaxThreads;
     }
-    if (other.timeout != 1800) {
+    if (isMergeAble(other.timeout, timeout)) {
       timeout = other.timeout;
     }
     // role, port, log, debug and help are not merged, they are only consumed by the immediately running node and can't affect a remote
   }
 
+  /**
+   * Determines if one object can be merged onto another object. Checks for {@code null},
+   * and empty (Collections & Maps) to make decision.
+   *
+   * @param other the object to merge. must be the same type as the 'target'
+   * @param target the object to merge on to. must be the same type as the 'other'
+   * @return whether the 'other' can be merged onto the 'target'
+   */
+  protected boolean isMergeAble(Object other, Object target) {
+    // don't merge a null value
+    if (other == null) {
+      return false;
+    } else {
+      // allow any non-null value to merge over a null target.
+      if (target == null) {
+        return true;
+      }
+    }
+
+    // we know we have two objects with value.. Make sure the types are the same and
+    // perform additional checks.
+
+    if (! target.getClass().getSuperclass().getTypeName()
+        .equals(other.getClass().getSuperclass().getTypeName())) {
+      return false;
+    }
+
+    if (target instanceof Collection) {
+      return !((Collection) other).isEmpty();
+    }
+
+    if (target instanceof Map) {
+      return !((Map) other).isEmpty();
+    }
+
+    return true;
+  }
+
   public String toString(String format) {
     StringBuilder sb = new StringBuilder();
     sb.append(toString(format, "browserTimeout", browserTimeout));
     sb.append(toString(format, "debug", debug));
     sb.append(toString(format, "help", help));
-    sb.append(toString(format, "jettyThreads", jettyThreads));
+    sb.append(toString(format, "jettyMaxThreads", jettyMaxThreads));
     sb.append(toString(format, "log", log));
     sb.append(toString(format, "logLongForm", logLongForm));
     sb.append(toString(format, "port", port));

diff --git a/java/server/src/org/openqa/grid/web/Hub.java b/java/server/src/org/openqa/grid/web/Hub.java
@@ -140,7 +140,7 @@ private void addDefaultServlets(ServletContextHandler handler) {
 
   private void initServer() {
     try {
-      if (config.jettyMaxThreads>0) {
+      if (config.jettyMaxThreads != null && config.jettyMaxThreads > 0) {
         QueuedThreadPool pool = new QueuedThreadPool();
         pool.setMaxThreads(config.jettyMaxThreads);
         server = new Server(pool);

diff --git a/java/server/src/org/openqa/selenium/remote/server/SeleniumServer.java b/java/server/src/org/openqa/selenium/remote/server/SeleniumServer.java
@@ -99,8 +99,8 @@ public void setExtraServlets(Map<String, Class<? extends Servlet>> extraServlets
   }
 
   public void boot() {
-    if (configuration.jettyThreads != null && configuration.jettyThreads > 0) {
-      server = new Server(new QueuedThreadPool(configuration.jettyThreads));
+    if (configuration.jettyMaxThreads != null && configuration.jettyMaxThreads > 0) {
+      server = new Server(new QueuedThreadPool(configuration.jettyMaxThreads));
     } else {
       server = new Server();
     }

diff --git a/java/server/test/org/openqa/grid/internal/BaseRemoteProxyTest.java b/java/server/test/org/openqa/grid/internal/BaseRemoteProxyTest.java
@@ -89,17 +89,37 @@ public void create() {
   public void proxyConfigIsInheritedFromRegistry() {
     Registry registry = Registry.newInstance();
     registry.getConfiguration().cleanUpCycle = 42;
-    registry.getConfiguration().timeout = 4200;
 
     GridNodeConfiguration nodeConfiguration = new GridNodeConfiguration();
-    new JCommander(nodeConfiguration, "-role", "webdriver", "-timeout", "100", "-cleanUpCycle", "100");
+    new JCommander(nodeConfiguration, "-role", "webdriver");
     RegistrationRequest req = RegistrationRequest.build(nodeConfiguration);
     req.getConfiguration().proxy = null;
 
     RemoteProxy p = BaseRemoteProxy.getNewInstance(req, registry);
 
-    assertEquals(42, p.getConfig().cleanUpCycle.longValue());
-    assertEquals(4200, p.getConfig().timeout.longValue());
+    // values which are not present in the registration request need to come
+    // from the registry
+    assertEquals(registry.getConfiguration().cleanUpCycle.longValue(),
+                 p.getConfig().cleanUpCycle.longValue());
+  }
+
+  @Test
+  public void proxyConfigOverwritesRegistryConfig() {
+    Registry registry = Registry.newInstance();
+    registry.getConfiguration().cleanUpCycle = 42;
+    registry.getConfiguration().maxSession = 1;
+
+    GridNodeConfiguration nodeConfiguration = new GridNodeConfiguration();
+    new JCommander(nodeConfiguration, "-role", "webdriver", "-cleanUpCycle", "100", "-maxSession", "50");
+    RegistrationRequest req = RegistrationRequest.build(nodeConfiguration);
+    req.getConfiguration().proxy = null;
+
+    RemoteProxy p = BaseRemoteProxy.getNewInstance(req, registry);
+
+    // values which are present in both the registration request and the registry need to
+    // come from the registration request
+    assertEquals(100L, p.getConfig().cleanUpCycle.longValue());
+    assertEquals(50L, p.getConfig().maxSession.longValue());
   }
 
   @Test