zookeeper-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From rake...@apache.org
Subject zookeeper git commit: ZOOKEEPER-2680: Correct DataNode.getChildren() inconsistent behaviour
Date Fri, 10 Feb 2017 17:28:44 GMT
Repository: zookeeper
Updated Branches:
  refs/heads/branch-3.4 286bf6913 -> 07a12868b


ZOOKEEPER-2680: Correct DataNode.getChildren() inconsistent behaviour

Author: Mohammad Arshad <arshad@apache.org>

Reviewers: Edward Ribeiro <edward.ribeiro@gmail.com>, Abraham Fine <afine@apache.org>,
Michael Han <hanm@apache.org>, Rakesh Radhakrishnan <rakeshr@apache.org>

Closes #162 from arshadmohammad/ZOOKEEPER-2680-br-3.4


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

Branch: refs/heads/branch-3.4
Commit: 07a12868bbfc16308b7bf192ac3abda77d1f2846
Parents: 286bf69
Author: Mohammad Arshad <arshad@apache.org>
Authored: Sat Feb 11 04:39:22 2017 +0530
Committer: Rakesh Radhakrishnan <rakeshr@apache.org>
Committed: Sat Feb 11 04:39:22 2017 +0530

----------------------------------------------------------------------
 .../org/apache/zookeeper/server/DataNode.java   |  4 +-
 .../org/apache/zookeeper/server/DataTree.java   | 48 +++++----------
 .../zookeeper/server/PrepRequestProcessor.java  |  3 +-
 .../zookeeper/server/SnapshotFormatter.java     |  6 +-
 .../apache/zookeeper/server/DataNodeTest.java   | 65 ++++++++++++++++++++
 5 files changed, 85 insertions(+), 41 deletions(-)
----------------------------------------------------------------------


http://git-wip-us.apache.org/repos/asf/zookeeper/blob/07a12868/src/java/main/org/apache/zookeeper/server/DataNode.java
----------------------------------------------------------------------
diff --git a/src/java/main/org/apache/zookeeper/server/DataNode.java b/src/java/main/org/apache/zookeeper/server/DataNode.java
index 4c79d55..8efdaf8 100644
--- a/src/java/main/org/apache/zookeeper/server/DataNode.java
+++ b/src/java/main/org/apache/zookeeper/server/DataNode.java
@@ -60,6 +60,8 @@ public class DataNode implements Record {
      */
     private Set<String> children = null;
 
+    private static final Set<String> EMPTY_SET = Collections.emptySet();
+
     /**
      * default constructor for the datanode
      */
@@ -130,7 +132,7 @@ public class DataNode implements Record {
      */
     public synchronized Set<String> getChildren() {
         if (children == null) {
-            return children;
+            return EMPTY_SET;
         }
 
         return Collections.unmodifiableSet(children);

http://git-wip-us.apache.org/repos/asf/zookeeper/blob/07a12868/src/java/main/org/apache/zookeeper/server/DataTree.java
----------------------------------------------------------------------
diff --git a/src/java/main/org/apache/zookeeper/server/DataTree.java b/src/java/main/org/apache/zookeeper/server/DataTree.java
index 25f2975..656e4e9 100644
--- a/src/java/main/org/apache/zookeeper/server/DataTree.java
+++ b/src/java/main/org/apache/zookeeper/server/DataTree.java
@@ -385,10 +385,8 @@ public class DataTree {
         }
         synchronized (parent) {
             Set<String> children = parent.getChildren();
-            if (children != null) {
-                if (children.contains(childName)) {
-                    throw new KeeperException.NodeExistsException();
-                }
+            if (children.contains(childName)) {
+                throw new KeeperException.NodeExistsException();
             }
             
             if (parentCVersion == -1) {
@@ -598,14 +596,7 @@ public class DataTree {
             if (stat != null) {
                 n.copyStat(stat);
             }
-            ArrayList<String> children;
-            Set<String> childs = n.getChildren();
-            if (childs != null) {
-                children = new ArrayList<String>(childs.size());
-                children.addAll(childs);
-            } else {
-                children = new ArrayList<String>(0);
-            }
+            List<String> children = new ArrayList<String>(n.getChildren());
 
             if (watcher != null) {
                 childWatches.addWatch(path, watcher);
@@ -937,17 +928,12 @@ public class DataTree {
         int len = 0;
         synchronized (node) {
             Set<String> childs = node.getChildren();
-            if (childs != null) {
-                children = childs.toArray(new String[childs.size()]);
-            }
+            children = childs.toArray(new String[childs.size()]);
             len = (node.data == null ? 0 : node.data.length);
         }
         // add itself
         counts.count += 1;
         counts.bytes += len;
-        if (children == null || children.length == 0) {
-            return;
-        }
         for (String child : children) {
             getCounts(path + "/" + child, counts);
         }
@@ -987,11 +973,9 @@ public class DataTree {
         String children[] = null;
         synchronized (node) {
             Set<String> childs = node.getChildren();
-            if (childs != null) {
-                children = childs.toArray(new String[childs.size()]);
-            }
+            children = childs.toArray(new String[childs.size()]);
         }
-        if (children == null || children.length == 0) {
+        if (children.length == 0) {
             // this node does not have a child
             // is the leaf node
             // check if its the leaf node
@@ -1051,23 +1035,19 @@ public class DataTree {
             //are never changed
             nodeCopy = new DataNode(node.parent, node.data, node.acl, statCopy);
             Set<String> childs = node.getChildren();
-            if (childs != null) {
-                children = childs.toArray(new String[childs.size()]);
-            }
+            children = childs.toArray(new String[childs.size()]);
         }
         oa.writeString(pathString, "path");
         oa.writeRecord(nodeCopy, "node");
         path.append('/');
         int off = path.length();
-        if (children != null) {
-            for (String child : children) {
-                // since this is single buffer being resused
-                // we need
-                // to truncate the previous bytes of string.
-                path.delete(off, Integer.MAX_VALUE);
-                path.append(child);
-                serializeNode(oa, path);
-            }
+        for (String child : children) {
+            // since this is single buffer being resused
+            // we need
+            // to truncate the previous bytes of string.
+            path.delete(off, Integer.MAX_VALUE);
+            path.append(child);
+            serializeNode(oa, path);
         }
     }
 

http://git-wip-us.apache.org/repos/asf/zookeeper/blob/07a12868/src/java/main/org/apache/zookeeper/server/PrepRequestProcessor.java
----------------------------------------------------------------------
diff --git a/src/java/main/org/apache/zookeeper/server/PrepRequestProcessor.java b/src/java/main/org/apache/zookeeper/server/PrepRequestProcessor.java
index fe02b8f..1248b08 100644
--- a/src/java/main/org/apache/zookeeper/server/PrepRequestProcessor.java
+++ b/src/java/main/org/apache/zookeeper/server/PrepRequestProcessor.java
@@ -154,8 +154,7 @@ public class PrepRequestProcessor extends ZooKeeperCriticalThread implements
                     synchronized(n) {
                         children = n.getChildren();
                     }
-                    lastChange = new ChangeRecord(-1, path, n.stat,
-                        children != null ? children.size() : 0,
+                    lastChange = new ChangeRecord(-1, path, n.stat, children.size(),
                             zks.getZKDatabase().aclForNode(n));
                 }
             }

http://git-wip-us.apache.org/repos/asf/zookeeper/blob/07a12868/src/java/main/org/apache/zookeeper/server/SnapshotFormatter.java
----------------------------------------------------------------------
diff --git a/src/java/main/org/apache/zookeeper/server/SnapshotFormatter.java b/src/java/main/org/apache/zookeeper/server/SnapshotFormatter.java
index f94c54d..bc43402 100644
--- a/src/java/main/org/apache/zookeeper/server/SnapshotFormatter.java
+++ b/src/java/main/org/apache/zookeeper/server/SnapshotFormatter.java
@@ -94,10 +94,8 @@ public class SnapshotFormatter {
             }
             children = n.getChildren();
         }
-        if (children != null) {
-            for (String child : children) {
-                printZnode(dataTree, name + (name.equals("/") ? "" : "/") + child);
-            }
+        for (String child : children) {
+            printZnode(dataTree, name + (name.equals("/") ? "" : "/") + child);
         }
     }
 

http://git-wip-us.apache.org/repos/asf/zookeeper/blob/07a12868/src/java/test/org/apache/zookeeper/server/DataNodeTest.java
----------------------------------------------------------------------
diff --git a/src/java/test/org/apache/zookeeper/server/DataNodeTest.java b/src/java/test/org/apache/zookeeper/server/DataNodeTest.java
new file mode 100644
index 0000000..6289766
--- /dev/null
+++ b/src/java/test/org/apache/zookeeper/server/DataNodeTest.java
@@ -0,0 +1,65 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.zookeeper.server;
+
+import static org.junit.Assert.*;
+
+import java.util.Set;
+
+import org.junit.Test;
+
+public class DataNodeTest {
+
+    @Test
+    public void testGetChildrenShouldReturnEmptySetWhenThereAreNoChidren() {
+        // create DataNode and call getChildren
+        DataNode dataNode = new DataNode();
+        Set<String> children = dataNode.getChildren();
+        assertNotNull(children);
+        assertEquals(0, children.size());
+
+        // add child,remove child and then call getChildren
+        String child = "child";
+        dataNode.addChild(child);
+        dataNode.removeChild(child);
+        children = dataNode.getChildren();
+        assertNotNull(children);
+        assertEquals(0, children.size());
+
+        // Returned empty set must not be modifiable
+        children = dataNode.getChildren();
+        try {
+            children.add("new child");
+            fail("UnsupportedOperationException is expected");
+        } catch (UnsupportedOperationException e) {
+            // do nothing
+        }
+    }
+
+    @Test
+    public void testGetChildrenReturnsImmutableEmptySet() {
+        DataNode dataNode = new DataNode();
+        Set<String> children = dataNode.getChildren();
+        try {
+            children.add("new child");
+            fail("UnsupportedOperationException is expected");
+        } catch (UnsupportedOperationException e) {
+            // do nothing
+        }
+    }
+}


Mime
View raw message