Mailing-List: contact commits-help@accumulo.apache.org; run by ezmlm
Precedence: bulk
Reply-To: dev@accumulo.apache.org
Content-Type: text/plain; charset="us-ascii"
MIME-Version: 1.0
Content-Transfer-Encoding: 7bit
From: elserj@apache.org
To: commits@accumulo.apache.org
Date: Fri, 22 Jul 2016 19:46:10 -0000
Message-Id: <f9108ebc4b9c434380f56ab1043bd172@git.apache.org>
In-Reply-To: <fdd60ec8609e4a14a5a439fda48d5403@git.apache.org>
References: <fdd60ec8609e4a14a5a439fda48d5403@git.apache.org>
Subject: [2/6] accumulo git commit: ACCUMULO-4388: Move location to obtain zoo
 session.
archived-at: Fri, 22 Jul 2016 19:46:12 -0000

ACCUMULO-4388: Move location to obtain zoo session.

Change the location of obtaining the zoo session. Tests within ZooCacheTest
appeared to cover these changes to ensure it did not break functionality; however
if there are ideas I'll be glad to provide additional tests.

Adding comments to make clear the reasoning behind pulling call
to getZooKeepers into run(), so that future developers will follow
the same plan when implementing ZooRunnable.

Closes apache/accumulo#133

Signed-off-by: Josh Elser <elserj@apache.org>


Project: http://git-wip-us.apache.org/repos/asf/accumulo/repo
Commit: http://git-wip-us.apache.org/repos/asf/accumulo/commit/cfebe561
Tree: http://git-wip-us.apache.org/repos/asf/accumulo/tree/cfebe561
Diff: http://git-wip-us.apache.org/repos/asf/accumulo/diff/cfebe561

Branch: refs/heads/1.8
Commit: cfebe5618fcbcca530cb52a1ed04a3ec201f6f8f
Parents: 9822860
Author: phrocker <marc.parisi@gmail.com>
Authored: Fri Jul 22 12:42:18 2016 -0400
Committer: Josh Elser <elserj@apache.org>
Committed: Fri Jul 22 15:08:52 2016 -0400

----------------------------------------------------------------------
 .../accumulo/fate/zookeeper/ZooCache.java       | 36 ++++++++++++++++----
 1 file changed, 29 insertions(+), 7 deletions(-)
----------------------------------------------------------------------


http://git-wip-us.apache.org/repos/asf/accumulo/blob/cfebe561/fate/src/main/java/org/apache/accumulo/fate/zookeeper/ZooCache.java
----------------------------------------------------------------------
diff --git a/fate/src/main/java/org/apache/accumulo/fate/zookeeper/ZooCache.java b/fate/src/main/java/org/apache/accumulo/fate/zookeeper/ZooCache.java
index 81985d7..503e56c 100644
--- a/fate/src/main/java/org/apache/accumulo/fate/zookeeper/ZooCache.java
+++ b/fate/src/main/java/org/apache/accumulo/fate/zookeeper/ZooCache.java
@@ -63,6 +63,14 @@ public class ZooCache {
 
   private final ZooReader zReader;
 
+  /**
+   * Returns a ZooKeeper session. Calls should be made within run of ZooRunnable after caches are checked. This will be performed at each retry of the run
+   * method. Calls to {@link #getZooKeeper()} should be made, ideally, after cache checks since other threads may have succeeded when updating the cache. Doing
+   * this will ensure that we don't pay the cost of retrieving a ZooKeeper session on each retry until we've ensured the caches aren't populated for a given
+   * node.
+   *
+   * @return ZooKeeper session.
+   */
   private ZooKeeper getZooKeeper() {
     return zReader.getZooKeeper();
   }
@@ -155,20 +163,30 @@ public class ZooCache {
 
   private abstract class ZooRunnable<T> {
     /**
-     * Runs an operation against ZooKeeper, automatically retrying in the face of KeeperExceptions
+     * Runs an operation against ZooKeeper. Retries are performed by the retry method when KeeperExceptions occur.
+     *
+     * Changes were made in ACCUMULO-4388 so that the run method no longer accepts Zookeeper as an argument, and instead relies on the ZooRunnable
+     * implementation to call {@link #getZooKeeper()}. Performing the call to retrieving a ZooKeeper Session after caches are checked has the benefit of
+     * limiting ZK connections and blocking as a result of obtaining these sessions.
+     *
+     * @return T the result of the runnable
      */
-    abstract T run(ZooKeeper zooKeeper) throws KeeperException, InterruptedException;
+    abstract T run() throws KeeperException, InterruptedException;
 
+    /**
+     * Retry will attempt to call the run method. Run should make a call to {@link #getZooKeeper()} after checks to cached information are made. This change,
+     * per ACCUMULO-4388 ensures that we don't create a ZooKeeper session when information is cached, and access to ZooKeeper is unnecessary.
+     *
+     * @return result of the runnable access success ( i.e. no exceptions ).
+     */
     public T retry() {
 
       int sleepTime = 100;
 
       while (true) {
 
-        ZooKeeper zooKeeper = getZooKeeper();
-
         try {
-          return run(zooKeeper);
+          return run();
         } catch (KeeperException e) {
           final Code code = e.code();
           if (code == Code.NONODE) {
@@ -210,7 +228,7 @@ public class ZooCache {
     ZooRunnable<List<String>> zr = new ZooRunnable<List<String>>() {
 
       @Override
-      public List<String> run(ZooKeeper zooKeeper) throws KeeperException, InterruptedException {
+      public List<String> run() throws KeeperException, InterruptedException {
         try {
           cacheReadLock.lock();
           if (childrenCache.containsKey(zPath)) {
@@ -225,6 +243,9 @@ public class ZooCache {
           if (childrenCache.containsKey(zPath)) {
             return childrenCache.get(zPath);
           }
+
+          final ZooKeeper zooKeeper = getZooKeeper();
+
           List<String> children = zooKeeper.getChildren(zPath, watcher);
           childrenCache.put(zPath, children);
           return children;
@@ -272,7 +293,7 @@ public class ZooCache {
     ZooRunnable<byte[]> zr = new ZooRunnable<byte[]>() {
 
       @Override
-      public byte[] run(ZooKeeper zooKeeper) throws KeeperException, InterruptedException {
+      public byte[] run() throws KeeperException, InterruptedException {
         Stat stat = null;
         cacheReadLock.lock();
         try {
@@ -292,6 +313,7 @@ public class ZooCache {
          */
         cacheWriteLock.lock();
         try {
+          final ZooKeeper zooKeeper = getZooKeeper();
           stat = zooKeeper.exists(zPath, watcher);
           byte[] data = null;
           if (stat == null) {