From 233ceb1d05d5ccb7d33c20bf969ddc75c7264ea3 Mon Sep 17 00:00:00 2001 From: Jay J Wylie Date: Thu, 4 Oct 2012 08:33:55 -0700 Subject: [PATCH] Removed the commented out implementations of distinct policies for async socket checkout. --- .../ClientRequestExecutorPool.java | 12 +--- .../utils/pool/AsyncResourceRequest.java | 3 - .../utils/pool/KeyedResourcePool.java | 51 ++------------- .../utils/pool/QueuedKeyedResourcePool.java | 64 +------------------ .../pool/QueuedKeyedResourcePoolTest.java | 8 --- 5 files changed, 10 insertions(+), 128 deletions(-) diff --git a/src/java/voldemort/store/socket/clientrequest/ClientRequestExecutorPool.java b/src/java/voldemort/store/socket/clientrequest/ClientRequestExecutorPool.java index 7a38b3087d..6f9450eea8 100644 --- a/src/java/voldemort/store/socket/clientrequest/ClientRequestExecutorPool.java +++ b/src/java/voldemort/store/socket/clientrequest/ClientRequestExecutorPool.java @@ -187,6 +187,8 @@ public void checkin(SocketDestination destination, ClientRequestExecutor clientR } } + // TODO: remove this helper method once JmxUtils are all updated to track + // queue stats of interest. /** * This method is useful for printing out QueuedKeyedResourcePool statistics * if debugging issues with the underlying QueuedKeyedResourcePool and @@ -261,7 +263,6 @@ private class AsyncSocketDestinationRequest implements public final String operationName; private final long startTimeNs; - private final long startTimeMs; public AsyncSocketDestinationRequest(SocketDestination destination, ClientRequest delegate, @@ -275,7 +276,6 @@ public AsyncSocketDestinationRequest(SocketDestination destination, this.operationName = operationName; this.startTimeNs = System.nanoTime(); - this.startTimeMs = System.currentTimeMillis(); } public void useResource(ClientRequestExecutor clientRequestExecutor) { @@ -328,14 +328,6 @@ public void handleException(Exception e) { public long getDeadlineNs() { return startTimeNs + TimeUnit.MILLISECONDS.toNanos(timeoutMs); } - - public long getStartTimeNs() { - return startTimeNs; - } - - public long getStartTimeMs() { - return startTimeMs; - } } private class NonblockingStoreCallbackClientRequest implements ClientRequest { diff --git a/src/java/voldemort/utils/pool/AsyncResourceRequest.java b/src/java/voldemort/utils/pool/AsyncResourceRequest.java index 5ae2f249f0..8bf0945cef 100644 --- a/src/java/voldemort/utils/pool/AsyncResourceRequest.java +++ b/src/java/voldemort/utils/pool/AsyncResourceRequest.java @@ -33,7 +33,4 @@ public interface AsyncResourceRequest { */ long getDeadlineNs(); - long getStartTimeNs(); - - long getStartTimeMs(); } diff --git a/src/java/voldemort/utils/pool/KeyedResourcePool.java b/src/java/voldemort/utils/pool/KeyedResourcePool.java index 1b12e624e0..0dfe843bed 100644 --- a/src/java/voldemort/utils/pool/KeyedResourcePool.java +++ b/src/java/voldemort/utils/pool/KeyedResourcePool.java @@ -11,7 +11,6 @@ import java.util.concurrent.TimeoutException; import java.util.concurrent.atomic.AtomicBoolean; import java.util.concurrent.atomic.AtomicInteger; -import java.util.concurrent.atomic.AtomicLong; import org.apache.log4j.Logger; @@ -107,7 +106,6 @@ public static KeyedResourcePool create(ResourceFactory factor public V checkout(K key) throws Exception { checkNotClosed(); - // TODO: clean up nanotime BS long startNs = System.nanoTime(); Pool resourcePool = getResourcePoolForKey(key); // Always attempt to grow. This protects against running out of @@ -123,7 +121,7 @@ public V checkout(K key) throws Exception { long timeRemainingNs = resourcePoolConfig.getTimeout(TimeUnit.NANOSECONDS) - (System.nanoTime() - startNs); if(timeRemainingNs > 0) - resource = resourcePool.blockingGet(timeRemainingNs, System.currentTimeMillis()); + resource = resourcePool.blockingGet(timeRemainingNs); if(resource == null) throw new TimeoutException("Could not acquire resource in " @@ -140,7 +138,7 @@ public V checkout(K key) throws Exception { return resource; } - /* + /** * Get a free resource if one exists. This method does not block. It either * returns null or a resource. */ @@ -149,7 +147,7 @@ protected V attemptCheckout(Pool pool) throws Exception { return resource; } - /* + /** * Attempt to create a new object and add it to the pool--this only happens * if there is room for the new object. This method does not block. This * method returns true if it adds a resource to the pool. (Returning true @@ -160,7 +158,7 @@ protected boolean attemptGrow(K key, Pool pool) throws Exception { return pool.attemptGrow(key, this.objectFactory); } - /* + /** * Get the pool for the given key. If no pool exists, create one. */ protected Pool getResourcePoolForKey(K key) { @@ -175,7 +173,7 @@ protected Pool getResourcePoolForKey(K key) { return resourcePool; } - /* + /** * Get the pool for the given key. If no pool exists, throw an exception. */ protected Pool getResourcePoolForExistingKey(K key) { @@ -379,7 +377,7 @@ public int getBlockingGetsCount() { return count; } - /* + /** * Check that the pool is not closed, and throw an IllegalStateException if * it is. */ @@ -388,24 +386,6 @@ protected void checkNotClosed() { throw new IllegalStateException("Pool is closed!"); } - // OPTION I: Attempt to provide FIFO between sync and async requests. - /*- - protected long getLastTimeMs(K key) { - long rc = 0; - if(!resourcePoolMap.containsKey(key)) { - return rc; - } - try { - Pool resourcePool = getResourcePoolForExistingKey(key); - rc = resourcePool.lastTimeMs.get(); - } catch(IllegalArgumentException iae) { - logger.debug("getBlockingGetsCount called on invalid key: ", iae); - } - return rc; - } - - // */ - /** * A fixed size pool that uses an ArrayBlockingQueue. The pool grows to no * more than some specified maxPoolSize. The pool creates new resources in @@ -416,7 +396,6 @@ protected static class Pool { final private AtomicInteger size = new AtomicInteger(0); final private AtomicInteger blockingGets = new AtomicInteger(0); - private final AtomicLong lastTimeMs = new AtomicLong(0); final private int maxPoolSize; final private BlockingQueue queue; @@ -470,14 +449,11 @@ public V nonBlockingGet() { return this.queue.poll(); } - public V blockingGet(long timeoutNs, long startTimeMs) throws InterruptedException { + public V blockingGet(long timeoutNs) throws InterruptedException { V v; try { blockingGets.incrementAndGet(); v = this.queue.poll(timeoutNs, TimeUnit.NANOSECONDS); - // OPTION I: Attempt to provide FIFO between sync and async - // requests. - // updateLastTimeMs(startTimeMs); } finally { blockingGets.decrementAndGet(); } @@ -494,18 +470,5 @@ public List close() { return list; } - // OPTION I: Attempt to provide FIFO between sync and async requests. - /*- - private void updateLastTimeMs(long startTimeMs) { - long curLastTimeMs = lastTimeMs.get(); - while(startTimeMs > curLastTimeMs) { - if(lastTimeMs.compareAndSet(curLastTimeMs, startTimeMs)) { - break; - } - curLastTimeMs = lastTimeMs.get(); - } - } - // */ - } } diff --git a/src/java/voldemort/utils/pool/QueuedKeyedResourcePool.java b/src/java/voldemort/utils/pool/QueuedKeyedResourcePool.java index be0c9c100b..7e516ca326 100644 --- a/src/java/voldemort/utils/pool/QueuedKeyedResourcePool.java +++ b/src/java/voldemort/utils/pool/QueuedKeyedResourcePool.java @@ -159,8 +159,6 @@ private boolean processQueue(K key) { try { // Always attempt to grow to deal with destroyed resources. attemptGrow(key, resourcePool); - // TODO: Concerned about mixing the bare poll() in attemptCheckout - // with queueing poll(timeout) in KeyedResourcePool... resource = attemptCheckout(resourcePool); } catch(Exception e) { destroyResource(key, resourcePool, resource); @@ -184,22 +182,9 @@ private boolean processQueue(K key) { } resourceRequest.useResource(resource); - // TODO: remove resourceRequest.getStartTimeNS() return true; } - private long peekNextStartTimeMs(K key) { - Queue> requestQueue = getRequestQueueForKey(key); - if(requestQueue.isEmpty()) { - return -1; - } - AsyncResourceRequest resourceRequest = requestQueue.peek(); - if(resourceRequest == null) { - return -1; - } - return resourceRequest.getStartTimeMs(); - } - /** * Attempts to repeatedly process enqueued resource requests. Tries until no * more progress is possible without blocking. @@ -218,61 +203,14 @@ private void processQueueLoop(K key) { */ @Override public void checkin(K key, V resource) throws Exception { - // OPTION I: Attempt to provide FIFO between sync and async requests. - /*- - long nextStartTime = peekNextStartTimeMs(key); - if(nextStartTime != -1) { - if(nextStartTime < getLastTimeMs(key)) { - if(isOpenAndValid(key, resource)) { - Queue> requestQueue = getRequestQueueForKey(key); - AsyncResourceRequest resourceRequest = getNextUnexpiredResourceRequest(requestQueue); - if(resourceRequest != null) { - resourceRequest.useResource(resource); - return; - } - } else { - resource = null; // twas destroyed - } - } - } - // */ - - // OPTION II: Strictly prefer async requests over sync requets. - /*- - if(isOpenAndValid(key, resource)) { - Queue> requestQueue = getRequestQueueForKey(key); - AsyncResourceRequest resourceRequest = getNextUnexpiredResourceRequest(requestQueue); - if(resourceRequest != null) { - resourceRequest.useResource(resource); - return; - } - } else { - // Must null out resource since a side effect of a failed call to - // isOpenAndValid is to call KeyedResourcePool::destroyResource - // which can only safely be invoked once because its finally clause - // decrements KeyedResourcePool's size. - resource = null; // twas destroyed - } - */ - - // For either Option I or II: only checkin if resource is not null, to - // avoid side-effect of invoking destroyResource multiple times on the - // same resource. - /*- - if(resource != null) - super.checkin(key, resource); - // */ - - // Option III: super.checkin(key, resource); - // NB: Blocking checkout calls for synchronous requests get the resource // checked in above before processQueueLoop() attempts checkout below. // There is therefore a risk that asynchronous requests will be starved. processQueueLoop(key); } - /* + /** * A safe wrapper to destroy the given resource request. */ protected void destroyRequest(AsyncResourceRequest resourceRequest) { diff --git a/test/unit/voldemort/utils/pool/QueuedKeyedResourcePoolTest.java b/test/unit/voldemort/utils/pool/QueuedKeyedResourcePoolTest.java index ac6bb662f3..02724cc3c7 100644 --- a/test/unit/voldemort/utils/pool/QueuedKeyedResourcePoolTest.java +++ b/test/unit/voldemort/utils/pool/QueuedKeyedResourcePoolTest.java @@ -459,13 +459,5 @@ public void handleException(Exception e) { public long getDeadlineNs() { return deadlineNs; } - - public long getStartTimeNs() { - return 0; - } - - public long getStartTimeMs() { - return 0; - } } }