Repository: zookeeper Updated Branches: refs/heads/branch-3.5 4bc78ee81 -> 91c400829
ZOOKEEPER-2680: Correct DataNode.getChildren() inconsistent behaviour Author: Mohammad Arshad <[email protected]> Reviewers: Edward Ribeiro <[email protected]>, Abraham Fine <[email protected]>, Michael Han <[email protected]>, Rakesh Radhakrishnan <[email protected]> Closes #161 from arshadmohammad/ZOOKEEPER-2680-br-3.5 Project: http://git-wip-us.apache.org/repos/asf/zookeeper/repo Commit: http://git-wip-us.apache.org/repos/asf/zookeeper/commit/91c40082 Tree: http://git-wip-us.apache.org/repos/asf/zookeeper/tree/91c40082 Diff: http://git-wip-us.apache.org/repos/asf/zookeeper/diff/91c40082 Branch: refs/heads/branch-3.5 Commit: 91c400829ef92110d17ca1f328dda4749aae57b0 Parents: 4bc78ee Author: Mohammad Arshad <[email protected]> Authored: Sat Feb 11 04:37:29 2017 +0530 Committer: Rakesh Radhakrishnan <[email protected]> Committed: Sat Feb 11 04:37:29 2017 +0530 ---------------------------------------------------------------------- .../org/apache/zookeeper/server/DataNode.java | 7 ++- .../org/apache/zookeeper/server/DataTree.java | 43 ++++--------- .../zookeeper/server/PrepRequestProcessor.java | 3 +- .../zookeeper/server/SnapshotFormatter.java | 6 +- .../apache/zookeeper/server/DataNodeTest.java | 65 ++++++++++++++++++++ 5 files changed, 86 insertions(+), 38 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/zookeeper/blob/91c40082/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 40cc8ea..1bad238 100644 --- a/src/java/main/org/apache/zookeeper/server/DataNode.java +++ b/src/java/main/org/apache/zookeeper/server/DataNode.java @@ -57,6 +57,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 */ @@ -122,11 +124,12 @@ public class DataNode implements Record { /** * convenience methods to get the children * - * @return the children of this datanode + * @return the children of this datanode. If the datanode has no children, empty + * set is returned */ 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/91c40082/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 57eb952..7bd3380 100644 --- a/src/java/main/org/apache/zookeeper/server/DataTree.java +++ b/src/java/main/org/apache/zookeeper/server/DataTree.java @@ -449,7 +449,7 @@ public class DataTree { } synchronized (parent) { Set<String> children = parent.getChildren(); - if (children != null && children.contains(childName)) { + if (children.contains(childName)) { throw new KeeperException.NodeExistsException(); } @@ -662,13 +662,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>(0); - } else { - children = new ArrayList<String>(childs); - } + List<String> children=new ArrayList<String>(n.getChildren()); if (watcher != null) { childWatches.addWatch(path, watcher); @@ -1025,17 +1019,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); } @@ -1075,11 +1064,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 @@ -1138,23 +1125,19 @@ public class DataTree { //are never changed nodeCopy = new DataNode(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/91c40082/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 b02ff38..4899c81 100644 --- a/src/java/main/org/apache/zookeeper/server/PrepRequestProcessor.java +++ b/src/java/main/org/apache/zookeeper/server/PrepRequestProcessor.java @@ -165,8 +165,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/91c40082/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/91c40082/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 + } + } +}
