From 61c11c7edf1472ae4c6b7323b7790356b5aa7e8b Mon Sep 17 00:00:00 2001
From: Jay J Wylie <jay.j.wylie@gmail.com>
Date: Mon, 15 Oct 2012 07:55:31 -0700
Subject: [PATCH] Cleaned up comments and TODOs from prior commits.

---
 .../voldemort/server/VoldemortServer.java     |  2 -
 test/common/voldemort/ServerTestUtils.java    | 42 +++++++++++++++++--
 .../voldemort/utils/ServerTestUtilsTest.java  |  5 ++-
 3 files changed, 43 insertions(+), 6 deletions(-)

diff --git a/src/java/voldemort/server/VoldemortServer.java b/src/java/voldemort/server/VoldemortServer.java
index e2741e5816..fd22f46f6f 100644
--- a/src/java/voldemort/server/VoldemortServer.java
+++ b/src/java/voldemort/server/VoldemortServer.java
@@ -91,8 +91,6 @@ public VoldemortServer(VoldemortConfig config) {
         this.services = createServices();
     }
 
-    // TODO: Should we use a different VoldemortServer construction depending on
-    // whether cluster is written previously!?!?! (cluster.xml issue)
     public VoldemortServer(VoldemortConfig config, Cluster cluster) {
         super(ServiceType.VOLDEMORT);
         this.voldemortConfig = config;
diff --git a/test/common/voldemort/ServerTestUtils.java b/test/common/voldemort/ServerTestUtils.java
index 1b87ae17f1..1da8a3994c 100644
--- a/test/common/voldemort/ServerTestUtils.java
+++ b/test/common/voldemort/ServerTestUtils.java
@@ -228,7 +228,11 @@ public static HttpStore getHttpStore(String storeName,
     }
 
     /**
-     * Return a free port as chosen by new ServerSocket(0)
+     * Return a free port as chosen by new ServerSocket(0).
+     * 
+     * There is no guarantee that the port returned will be free when the caller
+     * attempts to bind to the port. This is a time-of-check-to-time-of-use
+     * (TOCTOU) issue that cannot be avoided.
      */
     public static int findFreePort() {
         return findFreePorts(1)[0];
@@ -236,6 +240,10 @@ public static int findFreePort() {
 
     /**
      * Return an array of free ports as chosen by new ServerSocket(0)
+     * 
+     * There is no guarantee that the ports returned will be free when the
+     * caller attempts to bind to some returned port. This is a
+     * time-of-check-to-time-of-use (TOCTOU) issue that cannot be avoided.
      */
     public static int[] findFreePorts(int n) {
         logger.info("findFreePorts cannot guarantee that ports identified as free will still be free when used. This is effectively a TOCTOU issue. Expect intermittent BindException when \"free\" ports are used.");
@@ -669,8 +677,17 @@ public static VoldemortServer startVoldemortServer(SocketStoreFactory socketStor
                                                        VoldemortConfig config,
                                                        Cluster cluster) {
 
-        // TODO: Should this use VoldemortServer(config) instead!?!?!?
-        // (config.xml)
+        // TODO: Some tests that use this method fail intermittently with the
+        // following output:
+        //
+        // A successor version version() to this version() exists for key
+        // cluster.xml
+        // voldemort.versioning.ObsoleteVersionException: A successor version
+        // version() to this version() exists for key cluster.xml"
+        //
+        // Need to trace through the constructor VoldemortServer(VoldemortConfig
+        // config, Cluster cluster) to understand how this error is possible,
+        // and why it only happens intermittently.
         VoldemortServer server = new VoldemortServer(config, cluster);
         server.start();
 
@@ -742,6 +759,25 @@ protected static Cluster internalStartVoldemortCluster(int numServers,
         return cluster;
     }
 
+    /**
+     * This method wraps up work that is done in many different tests to set up
+     * some number of Voldemort servers in a cluster. This method masks an
+     * intermittent TOCTOU problem with the ports identified by
+     * {@link #findFreePorts(int)} not actually being free when a server needs
+     * to bind to them.
+     * 
+     * @param numServers
+     * @param voldemortServers
+     * @param partitionMap
+     * @param socketStoreFactory
+     * @param useNio
+     * @param clusterFile
+     * @param storeFile
+     * @param properties
+     * @return Cluster object that was used to successfully start all of the
+     *         servers.
+     * @throws IOException
+     */
     public static Cluster startVoldemortCluster(int numServers,
                                                 VoldemortServer[] voldemortServers,
                                                 int[][] partitionMap,
diff --git a/test/unit/voldemort/utils/ServerTestUtilsTest.java b/test/unit/voldemort/utils/ServerTestUtilsTest.java
index 6b5f43776d..5608a6e605 100644
--- a/test/unit/voldemort/utils/ServerTestUtilsTest.java
+++ b/test/unit/voldemort/utils/ServerTestUtilsTest.java
@@ -62,7 +62,9 @@ public void startMultipleVoldemortClusters() throws IOException {
         }
     }
 
+    // **********************************************************************
     // * START : TESTS THAT HELPED FIND ROOT CAUSE OF BindException PROBLEM *
+
     // @Test
     public void startMultipleVoldemortServers() throws IOException {
         Cluster cluster = ServerTestUtils.getLocalCluster(8, new int[][] { { 0 }, { 1 }, { 2 },
@@ -177,6 +179,7 @@ public void testFindFreePorts100() throws Exception {
             testFindFreePorts();
         }
     }
-    // ** END : TESTS THAT HELPED FIND ROOT CAUSE OF BindException PROBLEM **
 
+    // ** END : TESTS THAT HELPED FIND ROOT CAUSE OF BindException PROBLEM **
+    // **********************************************************************
 }