Added unit test for QueuedKeyedResourcePool. Minor clean up of

QueuedKeyedResourcePool and of KeyedResourcePoolTest. Still one
outstanding TODO in QueuedKeyedResourcePool wrt semantics of
close(K key) method.

src/java/voldemort/utils/pool/QueuedKeyedResourcePool.java

@@ -14,8 +14,21 @@
  * Extends simple implementation of a per-key resource pool with a non-blocking
  * interface to enqueue requests for a resource when one becomes available. <br>
  * <ul>
- * <li>allocates resources in FIFO order
- * <li>Pools are per key and there is no global maximum pool limit.
+ * <li>Allocates resources in FIFOish order: blocking requests via checkout are
+ * FIFO and non-blocking enqueued requests are FIFO, however, there is no
+ * ordering between blocking (checkout) and non-blocking (requestResource).
+ * <li>Pools and Queues are per key and there is no global maximum pool or queue
+ * limit.
+ * </ul>
+ * 
+ * Beyond the expectations documented in KeyedResourcePool, the following is
+ * expected of the user of this class:
+ * <ul>
+ * <li>A resource acquired via {@link #checkout(K)) checkout} or via {@link
+ * #requestResource(K , ResourceRequest<V>) requestResource} is checked in
+ * exactly once.
+ * <li>A resource that is checked in was previously checked out or requested.
+ * <li>Also, reqeustResource is never called after close.
  * </ul>
  */
 public class QueuedKeyedResourcePool<K, V> extends KeyedResourcePool<K, V> {
@@ -112,9 +125,6 @@ public void requestResource(K key, ResourceRequest<V> resourceRequest) {
                 resourceRequest.handleException(e);
             }
             if(resource != null) {
-                // TODO: Is another try/catch block needed anywhere to ensure
-                // resource is destroyed if/when anything bad happens in
-                // useResource method?
                 resourceRequest.useResource(resource);
                 return;
             }
@@ -191,11 +201,9 @@ private boolean processQueue(K key) throws Exception {
      */
     @Override
     public void checkin(K key, V resource) throws Exception {
-        // TODO: Unclear if invoking checkin and then invoking processQueue is
-        // "fair" or is "FIFO". In particular, is super.checkout invoked
-        // directly? If so, how should such a blocking checkout interact with
-        // non-blocking checkouts?
         super.checkin(key, resource);
+        // NB: Blocking checkout calls may get checked in resource before
+        // processQueue.
         while(processQueue(key)) {}
     }
 

test/unit/voldemort/utils/pool/KeyedResourcePoolTest.java

@@ -21,12 +21,12 @@
 
 public class KeyedResourcePoolTest {
 
-    private static int POOL_SIZE = 5;
-    private static long TIMEOUT_MS = 100;
+    protected static int POOL_SIZE = 5;
+    protected static long TIMEOUT_MS = 100;
 
-    private TestResourceFactory factory;
-    private KeyedResourcePool<String, TestResource> pool;
-    private ResourcePoolConfig config;
+    protected TestResourceFactory factory;
+    protected KeyedResourcePool<String, TestResource> pool;
+    protected ResourcePoolConfig config;
 
     @Before
     public void setUp() {
@@ -140,6 +140,7 @@ public void testExceptionOnCreate() throws Exception {
         assertEquals(0, this.pool.getCheckedInResourceCount());
     }
 
+    // NEGATIVE Test. See comment in line.
     @Test
     public void repeatedCheckins() throws Exception {
         assertEquals(0, this.pool.getTotalResourceCount());
@@ -164,6 +165,7 @@ public void repeatedCheckins() throws Exception {
         // assertEquals(1, this.pool.getCheckedInResourceCount());
     }
 
+    // NEGATIVE Test. See comment in line.
     @Test
     public void testExceptionOnFullCheckin() throws Exception {
         assertEquals(0, this.pool.getTotalResourceCount());
@@ -196,6 +198,7 @@ public void testExceptionOnFullCheckin() throws Exception {
         // assertEquals(POOL_SIZE, this.pool.getTotalResourceCount());
     }
 
+    // NEGATIVE Test. See comment in line.
     @Test
     public void testCheckinExtraneousResource() throws Exception {
         assertEquals(0, this.pool.getTotalResourceCount());
@@ -212,6 +215,7 @@ public void testCheckinExtraneousResource() throws Exception {
         assertEquals(2, this.pool.getCheckedInResourceCount());
     }
 
+    // NEGATIVE Test. See comment in line.
     @Test
     public void testNeverCheckin() throws Exception {
         assertEquals(0, this.pool.getTotalResourceCount());
@@ -344,7 +348,7 @@ public void run() {
         }
     }
 
-    private static class TestResource {
+    protected static class TestResource {
 
         private String value;
         private AtomicBoolean isValid;
@@ -379,7 +383,7 @@ public String toString() {
 
     }
 
-    private static class TestResourceFactory implements ResourceFactory<String, TestResource> {
+    protected static class TestResourceFactory implements ResourceFactory<String, TestResource> {
 
         private final AtomicInteger created = new AtomicInteger(0);
         private final AtomicInteger destroyed = new AtomicInteger(0);