[grid] Reworking last *Flags classes to show defaults properly.

This is a completes the work needed that fixes issue #9216.
SeleniumHQ · Mar 6, 2021 · 30ae67a · 30ae67a
1 parent 0a19bc4
commit 30ae67a
Show file tree

Hide file tree

Showing 12 changed files with 101 additions and 80 deletions.
diff --git a/java/server/src/org/openqa/selenium/grid/distributor/config/DistributorFlags.java b/java/server/src/org/openqa/selenium/grid/distributor/config/DistributorFlags.java
@@ -46,7 +46,7 @@ public class DistributorFlags implements HasRoles {
     names = "--distributor-port",
     description = "Port on which the distributor is listening.")
   @ConfigValue(section = DISTRIBUTOR_SECTION, name = "port", example = "5553")
-  private int distributorServerPort;
+  private Integer distributorServerPort;
 
   @Parameter(
     names = "--distributor-host",

diff --git a/java/server/src/org/openqa/selenium/grid/router/httpd/RouterFlags.java b/java/server/src/org/openqa/selenium/grid/router/httpd/RouterFlags.java
@@ -17,27 +17,30 @@
 
 package org.openqa.selenium.grid.router.httpd;
 
-import com.beust.jcommander.Parameter;
+import static org.openqa.selenium.grid.config.StandardGridRoles.ROUTER_ROLE;
+
 import com.google.auto.service.AutoService;
+
+import com.beust.jcommander.Parameter;
+
 import org.openqa.selenium.grid.config.ConfigValue;
 import org.openqa.selenium.grid.config.HasRoles;
 import org.openqa.selenium.grid.config.Role;
 
 import java.util.Collections;
 import java.util.Set;
 
-import static org.openqa.selenium.grid.config.StandardGridRoles.ROUTER_ROLE;
-
 @AutoService(HasRoles.class)
 public class RouterFlags implements HasRoles {
 
+  @SuppressWarnings("FieldMayBeFinal")
   @Parameter(
     names = {"--relax-checks"},
     description = "Relax checks on origin header and content type of incoming requests," +
-      " in contravention of strict W3C spec compliance.",
+                  " in contravention of strict W3C spec compliance.",
     arity = 1)
   @ConfigValue(section = "network", name = "relax-checks", example = "true")
-  private Boolean relaxChecks;
+  private Boolean relaxChecks = false;
 
   @Override
   public Set<Role> getRoles() {

diff --git a/java/server/src/org/openqa/selenium/grid/server/EventBusFlags.java b/java/server/src/org/openqa/selenium/grid/server/EventBusFlags.java
@@ -17,37 +17,43 @@
 
 package org.openqa.selenium.grid.server;
 
-import com.beust.jcommander.Parameter;
+import static org.openqa.selenium.grid.config.StandardGridRoles.EVENT_BUS_ROLE;
+import static org.openqa.selenium.grid.server.EventBusOptions.EVENTS_SECTION;
+
 import com.google.auto.service.AutoService;
 import com.google.common.collect.ImmutableSet;
+
+import com.beust.jcommander.Parameter;
+
 import org.openqa.selenium.grid.config.ConfigValue;
 import org.openqa.selenium.grid.config.HasRoles;
 import org.openqa.selenium.grid.config.Role;
 
 import java.util.Set;
 
-import static org.openqa.selenium.grid.config.StandardGridRoles.EVENT_BUS_ROLE;
-
 @AutoService(HasRoles.class)
 public class EventBusFlags implements HasRoles {
 
   @Parameter(
-      names = {"--publish-events"},
-      description =  "Connection string for publishing events to the event bus")
-  @ConfigValue(section = "events", name = "publish", example = "\"tcp://*:1233\"")
+    names = {"--publish-events"},
+    description = "Connection string for publishing events to the event bus")
+  @ConfigValue(section = EVENTS_SECTION, name = "publish", example = "\"tcp://*:1233\"")
   private String publishString;
 
   @Parameter(
-      names = {"--subscribe-events"},
-      description =  "Connection string for subscribing to events from the event bus")
-  @ConfigValue(section = "events", name = "subscribe", example = "\"tcp://*1232\"")
+    names = {"--subscribe-events"},
+    description = "Connection string for subscribing to events from the event bus")
+  @ConfigValue(section = EVENTS_SECTION, name = "subscribe", example = "\"tcp://*1232\"")
   private String subscribeString;
 
   @Parameter(
-      names = {"--bind-bus"},
-      description = "Whether the connection string should be bound or connected",
-      arity = 1)
-  @ConfigValue(section = "events", name = "bind", example = "false")
+    names = {"--bind-bus"},
+    description = "Whether the connection string should be bound or connected. When true, the "
+                  + "component will be bound to the Event Bus (as in the Event Bus will also be "
+                  + "started by the component, typically by the Distributor and the Hub). When "
+                  + "false, the component will connect to the Event Bus.",
+    arity = 1)
+  @ConfigValue(section = EVENTS_SECTION, name = "bind", example = "false")
   // We use the Boolean here so we can differentiate between there being no option, and a default
   // false value.
   private Boolean bind;
@@ -56,7 +62,7 @@ public class EventBusFlags implements HasRoles {
       names = {"--events-implementation"},
       description = "Full classname of non-default event bus implementation")
   @ConfigValue(
-    section = "events",
+    section = EVENTS_SECTION,
     name = "implementation",
     example = "org.openqa.selenium.events.zeromq.ZeroMqEventBus")
   private String implementation;

diff --git a/java/server/src/org/openqa/selenium/grid/server/EventBusOptions.java b/java/server/src/org/openqa/selenium/grid/server/EventBusOptions.java
@@ -23,8 +23,8 @@
 
 public class EventBusOptions {
 
+  static final String EVENTS_SECTION = "events";
   private static final String DEFAULT_CLASS = "org.openqa.selenium.events.zeromq.ZeroMqEventBus";
-
   private final Config config;
   private volatile EventBus bus;
 
@@ -48,6 +48,6 @@ public EventBus getEventBus() {
   }
 
   private EventBus createBus() {
-    return config.getClass("events", "implementation", EventBus.class, DEFAULT_CLASS);
+    return config.getClass(EVENTS_SECTION, "implementation", EventBus.class, DEFAULT_CLASS);
   }
 }
diff --git a/java/server/src/org/openqa/selenium/grid/sessionmap/config/SessionMapFlags.java b/java/server/src/org/openqa/selenium/grid/sessionmap/config/SessionMapFlags.java
@@ -17,8 +17,12 @@
 
 package org.openqa.selenium.grid.sessionmap.config;
 
-import com.beust.jcommander.Parameter;
+import static org.openqa.selenium.grid.config.StandardGridRoles.SESSION_MAP_ROLE;
+
 import com.google.auto.service.AutoService;
+
+import com.beust.jcommander.Parameter;
+
 import org.openqa.selenium.grid.config.ConfigValue;
 import org.openqa.selenium.grid.config.HasRoles;
 import org.openqa.selenium.grid.config.Role;
@@ -27,8 +31,6 @@
 import java.util.Collections;
 import java.util.Set;
 
-import static org.openqa.selenium.grid.config.StandardGridRoles.SESSION_MAP_ROLE;
-
 @AutoService(HasRoles.class)
 public class SessionMapFlags implements HasRoles {
 
@@ -37,10 +39,10 @@ public class SessionMapFlags implements HasRoles {
   private URI sessionServer;
 
   @Parameter(
-      names = "--sessions-port",
-      description = "Port on which the session map server is listening.")
+    names = "--sessions-port",
+    description = "Port on which the session map server is listening.")
   @ConfigValue(section = "sessions", name = "port", example = "1234")
-  private int sessionServerPort;
+  private Integer sessionServerPort;
 
   @Parameter(
       names = "--sessions-host",

diff --git a/java/server/src/org/openqa/selenium/grid/sessionmap/config/SessionMapOptions.java b/java/server/src/org/openqa/selenium/grid/sessionmap/config/SessionMapOptions.java
@@ -24,14 +24,13 @@
 import java.net.URI;
 import java.net.URISyntaxException;
 import java.util.Optional;
-import java.util.logging.Logger;
 
 public class SessionMapOptions {
 
   private static final String SESSIONS_SECTION = "sessions";
 
-  private static final Logger LOG = Logger.getLogger(SessionMapOptions.class.getName());
-  private static final String DEFAULT_SESSION_MAP = "org.openqa.selenium.grid.sessionmap.remote.RemoteSessionMap";
+  private static final String DEFAULT_SESSION_MAP =
+    "org.openqa.selenium.grid.sessionmap.remote.RemoteSessionMap";
   private static final String DEFAULT_SESSION_MAP_SCHEME = "http";
   private final Config config;
 
@@ -41,7 +40,8 @@ public SessionMapOptions(Config config) {
 
   public URI getSessionMapUri() {
 
-    String scheme = config.get(SESSIONS_SECTION, "scheme").orElse(DEFAULT_SESSION_MAP_SCHEME);
+    String scheme = config.get(SESSIONS_SECTION, "scheme")
+      .orElse(DEFAULT_SESSION_MAP_SCHEME);
 
     Optional<URI> host = config.get(SESSIONS_SECTION, "host").map(str -> {
       try {
@@ -80,6 +80,10 @@ public URI getSessionMapUri() {
   }
 
   public SessionMap getSessionMap() {
-    return config.getClass(SESSIONS_SECTION, "implementation", SessionMap.class, DEFAULT_SESSION_MAP);
+    return config.getClass(
+      SESSIONS_SECTION,
+      "implementation",
+      SessionMap.class,
+      DEFAULT_SESSION_MAP);
   }
 }
diff --git a/java/server/src/org/openqa/selenium/grid/sessionmap/jdbc/JdbcSessionMapFlags.java b/java/server/src/org/openqa/selenium/grid/sessionmap/jdbc/JdbcSessionMapFlags.java
@@ -17,31 +17,36 @@
 
 package org.openqa.selenium.grid.sessionmap.jdbc;
 
+import static org.openqa.selenium.grid.config.StandardGridRoles.SESSION_MAP_ROLE;
+
 import com.google.auto.service.AutoService;
+
 import com.beust.jcommander.Parameter;
+
 import org.openqa.selenium.grid.config.ConfigValue;
 import org.openqa.selenium.grid.config.HasRoles;
 import org.openqa.selenium.grid.config.Role;
 
 import java.util.Collections;
 import java.util.Set;
 
-import static org.openqa.selenium.grid.config.StandardGridRoles.SESSION_MAP_ROLE;
-
 
 @AutoService(HasRoles.class)
 public class JdbcSessionMapFlags implements HasRoles {
 
   @Parameter(
-      names = "--jdbc-url",
-      description = "Database URL for making a connection.")
-  @ConfigValue(section = "sessions", name = "jdbc-url", example = "\"jdbc:mysql://localhost:3306/TestDatabase\"")
+    names = "--jdbc-url",
+    description = "Database URL for making a connection.")
+  @ConfigValue(
+    section = "sessions",
+    name = "jdbc-url",
+    example = "\"jdbc:mysql://localhost:3306/TestDatabase\"")
   private String jdbcUrl;
 
   @Parameter(
-      names = "--jdbc-user",
-      description = "Username for the user to make a JDBC connection")
-  @ConfigValue(section = "sessions", name = "jdbc-user", example = "mytestUser")
+    names = "--jdbc-user",
+    description = "Username for the user to make a JDBC connection")
+  @ConfigValue(section = "sessions", name = "jdbc-user", example = "myTestUser")
   private String username;
 
   @Parameter(

diff --git a/java/server/src/org/openqa/selenium/grid/sessionmap/jdbc/JdbcSessionMapOptions.java b/java/server/src/org/openqa/selenium/grid/sessionmap/jdbc/JdbcSessionMapOptions.java
@@ -24,35 +24,31 @@
 import java.sql.DriverManager;
 import java.sql.SQLException;
 import java.util.NoSuchElementException;
-import java.util.logging.Logger;
 
 public class JdbcSessionMapOptions {
 
   private static final String SESSIONS_SECTION = "sessions";
-  private static final Logger LOG = Logger.getLogger(JdbcSessionMapOptions.class.getName());
 
-  private String jdbcUrl;
-  private String jdbcUser;
-  private String jdbcPassword;
-
-  private final Config config;
+  private final String jdbcUrl;
+  private final String jdbcUser;
+  private final String jdbcPassword;
 
   public JdbcSessionMapOptions(Config config) {
     Require.nonNull("Config", config);
 
-    this.config = config;
     try {
-      this.jdbcUrl = config.get(SESSIONS_SECTION, "jdbc-url").get();
-      this.jdbcUser = config.get(SESSIONS_SECTION, "jdbc-user").get();
-      this.jdbcPassword = config.get(SESSIONS_SECTION, "jdbc-password").get();
+      this.jdbcUrl = config.get(SESSIONS_SECTION, "jdbc-url").orElse("");
+      this.jdbcUser = config.get(SESSIONS_SECTION, "jdbc-user").orElse("");
+      this.jdbcPassword = config.get(SESSIONS_SECTION, "jdbc-password").orElse("");
 
       if (jdbcUrl.isEmpty()) {
         throw new JdbcException(
-            "Missing JDBC Url value. Add sessions option value --jdbc-url <url-value>");
+          "Missing JDBC Url value. Add sessions option value --jdbc-url <url-value>");
       }
     } catch (NoSuchElementException e) {
       throw new JdbcException(
-          "Missing session options. Check and add all the following options \n --jdbc-url <url> \n --jdbc-user <user> \n --jdbc-password <password>");
+        "Missing session options. Check and add all the following options \n "
+        + "--jdbc-url <url> \n --jdbc-user <user> \n --jdbc-password <password>");
     }
   }
 

diff --git a/java/server/src/org/openqa/selenium/grid/sessionqueue/config/NewSessionQueueFlags.java b/java/server/src/org/openqa/selenium/grid/sessionqueue/config/NewSessionQueueFlags.java
@@ -18,6 +18,9 @@
 package org.openqa.selenium.grid.sessionqueue.config;
 
 import static org.openqa.selenium.grid.config.StandardGridRoles.SESSION_QUEUE_ROLE;
+import static org.openqa.selenium.grid.sessionqueue.config.NewSessionQueueOptions.DEFAULT_REQUEST_TIMEOUT;
+import static org.openqa.selenium.grid.sessionqueue.config.NewSessionQueueOptions.DEFAULT_RETRY_INTERVAL;
+import static org.openqa.selenium.grid.sessionqueue.config.NewSessionQueueOptions.SESSIONS_QUEUE_SECTION;
 
 import com.google.auto.service.AutoService;
 
@@ -30,22 +33,23 @@
 import java.util.Collections;
 import java.util.Set;
 
+@SuppressWarnings("FieldMayBeFinal")
 @AutoService(HasRoles.class)
 public class NewSessionQueueFlags implements HasRoles {
 
   @Parameter(
-    names = { "--session-request-timeout" },
+    names = {"--session-request-timeout"},
     description = "Timeout in seconds. New incoming session request is added to the queue. "
-      + "Requests sitting in the queue for longer than the configured time will timeout.")
-  @ConfigValue(section = "sessionqueue", name = "session-request-timeout", example = "5")
-  private int sessionRequestTimeout = 300;
+                  + "Requests sitting in the queue for longer than the configured time will timeout.")
+  @ConfigValue(section = SESSIONS_QUEUE_SECTION, name = "session-request-timeout", example = "5")
+  private int sessionRequestTimeout = DEFAULT_REQUEST_TIMEOUT;
 
   @Parameter(
-    names = { "--session-retry-interval" },
+    names = {"--session-retry-interval"},
     description = "Retry interval in seconds. If all slots are busy, new session request " +
-      "will be retried after the given interval.")
-  @ConfigValue(section = "sessionqueue", name = "session-retry-interval", example = "5")
-  private int sessionRetryInterval = 5;
+                  "will be retried after the given interval.")
+  @ConfigValue(section = SESSIONS_QUEUE_SECTION, name = "session-retry-interval", example = "5")
+  private int sessionRetryInterval = DEFAULT_RETRY_INTERVAL;
 
   @Override
   public Set<Role> getRoles() {

diff --git a/java/server/src/org/openqa/selenium/grid/sessionqueue/config/NewSessionQueueOptions.java b/java/server/src/org/openqa/selenium/grid/sessionqueue/config/NewSessionQueueOptions.java
@@ -29,13 +29,11 @@
   description = "New session queue config")
 public class NewSessionQueueOptions {
 
-  private static final String SESSIONS_QUEUE_SECTION = "sessionqueue";
+  static final String SESSIONS_QUEUE_SECTION = "sessionqueue";
+  static final int DEFAULT_REQUEST_TIMEOUT = 300;
+  static final int DEFAULT_RETRY_INTERVAL = 5;
   private static final String DEFAULT_NEWSESSION_QUEUE =
     "org.openqa.selenium.grid.sessionmap.remote.LocalNewSessionQueue";
-  private static final int DEFAULT_REQUEST_TIMEOUT = 300;
-  private static final int DEFAULT_RETRY_INTERVAL = 5;
-
-
   private final Config config;
 
   public NewSessionQueueOptions(Config config) {
@@ -44,22 +42,21 @@ public NewSessionQueueOptions(Config config) {
   }
 
   public Duration getSessionRequestTimeout() {
-    long timeout = config.getInt(SESSIONS_QUEUE_SECTION, "session-request-timeout")
-      .orElse(DEFAULT_REQUEST_TIMEOUT);
+    // If the user sets 0 or less, we default to 1s.
+    int timeout = Math.max(
+      config.getInt(SESSIONS_QUEUE_SECTION, "session-request-timeout")
+        .orElse(DEFAULT_REQUEST_TIMEOUT),
+      1);
 
-    if (timeout <= 0) {
-      return Duration.ofSeconds(DEFAULT_REQUEST_TIMEOUT);
-    }
     return Duration.ofSeconds(timeout);
   }
 
   public Duration getSessionRequestRetryInterval() {
-    long interval = config.getInt(SESSIONS_QUEUE_SECTION, "session-retry-interval")
-      .orElse(DEFAULT_RETRY_INTERVAL);
-
-    if (interval <= 0) {
-      return Duration.ofSeconds(DEFAULT_RETRY_INTERVAL);
-    }
+    // If the user sets 0 or less, we default to 1s.
+    int interval = Math.max(
+      config.getInt(SESSIONS_QUEUE_SECTION, "session-retry-interval")
+        .orElse(DEFAULT_REQUEST_TIMEOUT),
+      1);
     return Duration.ofSeconds(interval);
   }
 

diff --git a/java/server/src/org/openqa/selenium/grid/sessionqueue/config/NewSessionQueuerFlags.java b/java/server/src/org/openqa/selenium/grid/sessionqueue/config/NewSessionQueuerFlags.java
@@ -44,7 +44,7 @@ public class NewSessionQueuerFlags implements HasRoles {
     names = "--sessionqueuer-port",
     description = "Port on which the session queue server is listening.")
   @ConfigValue(section = "sessionqueuer", name = "port", example = "1234")
-  private int sessionQueueServerPort;
+  private Integer sessionQueueServerPort;
 
   @Parameter(
     names = "--sessionqueuer-host",