[GitHub] QiuMM commented on a change in pull request #6263: Add ability to specify list of task ports

[GitBox <gi...@apache.org>]

QiuMM commented on a change in pull request #6263: Add ability to specify list of task ports
URL: https://github.com/apache/incubator-druid/pull/6263#discussion_r213758356
 
 

 ##########
 File path: indexing-service/src/main/java/io/druid/indexing/overlord/PortFinder.java
 ##########
 @@ -55,30 +58,54 @@ private static boolean canBind(int portNum)
 
   public synchronized int findUnusedPort()
   {
-    int port = chooseNext(startPort);
-    while (!canBind(port)) {
-      port = chooseNext(port + 1);
+    if (candidatePorts != null && !candidatePorts.isEmpty()) {
+      int port = chooseFromCandidates();
+      usedPorts.add(port);
+      return port;
+    } else {
+      int port = chooseNext(startPort);
+      while (!canBind(port)) {
+        port = chooseNext(port + 1);
+      }
+      usedPorts.add(port);
+      return port;
     }
-    usedPorts.add(port);
-    return port;
   }
 
-  public synchronized Pair<Integer, Integer> findTwoConsecutiveUnusedPorts()
+  public synchronized Pair<Integer, Integer> findTwoUnusedPorts()
   {
-    int firstPort = chooseNext(startPort);
-    while (!canBind(firstPort) || !canBind(firstPort + 1)) {
-      firstPort = chooseNext(firstPort + 1);
+    if (candidatePorts != null && !candidatePorts.isEmpty()) {
+      int firstPort = chooseFromCandidates();
+      int secondPort = chooseFromCandidates();
+      usedPorts.add(firstPort);
+      usedPorts.add(secondPort);
+      return new Pair<>(firstPort, secondPort);
+    } else {
+      int firstPort = chooseNext(startPort);
+      while (!canBind(firstPort) || !canBind(firstPort + 1)) {
+        firstPort = chooseNext(firstPort + 1);
+      }
+      usedPorts.add(firstPort);
+      usedPorts.add(firstPort + 1);
+      return new Pair<>(firstPort, firstPort + 1);
     }
-    usedPorts.add(firstPort);
-    usedPorts.add(firstPort + 1);
-    return new Pair<>(firstPort, firstPort + 1);
   }
 
   public synchronized void markPortUnused(int port)
   {
     usedPorts.remove(port);
   }
 
+  private int chooseFromCandidates()
+  {
+    for (int port : candidatePorts) {
+      if (!usedPorts.contains(port) && canBind(port)) {
+        return port;
+      }
+    }
+    throw new ISE("All ports are Used..");
 
 Review comment:
   I do not think so. The reason why adding ability to specify list of task ports is that we need to decide ahead of time exactly what ports peon processes are going to use in some containerized environments. So if user specify `druid.indexer.runner.ports`, they shoulde ensure ports are usable. And it may be a illegal thing in users' environments if we use `chooseNext()` to find a port while all the candidate ports are unusable. In my use case, it does a illegal thing and may cause potential port conflicts.

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org