You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@zookeeper.apache.org by arshadmohammad <gi...@git.apache.org> on 2017/01/30 20:16:26 UTC
[GitHub] zookeeper pull request #160: ZOOKEEPER-2680:Correct DataNode.getChildren() i...
GitHub user arshadmohammad opened a pull request:
https://github.com/apache/zookeeper/pull/160
ZOOKEEPER-2680:Correct DataNode.getChildren() inconsistent behaviour.
DataNode.getChildren() API behavior should be changed and it should always return empty set if the node does not have any child. Currently this API returns some times null some times empty set.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/arshadmohammad/zookeeper ZOOKEEPER-2680-DataNode-getChildren
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/zookeeper/pull/160.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #160
----
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zookeeper issue #160: ZOOKEEPER-2680:Correct DataNode.getChildren() inconsis...
Posted by eribeiro <gi...@git.apache.org>.
Github user eribeiro commented on the issue:
https://github.com/apache/zookeeper/pull/160
@arshadmohammad My two last questions (really!):
* Is this change supposed to be applied to branch-3.4 and branch-3.5?
* I see that `DataNode.getChildren()` is called in half a dozen places with more or less the logic below:
```
Set<String> childs = node.getChildren();
if (childs != null) {
children = childs.toArray(new String[childs.size()]);
}
// children size is zero
if (children != null) {
for (String child : children) {
// DO LOGIC
}
}
```
As `children` is zero length the for-each loop is not executed. I am fine with leaving this code as-is. Otherwise, we could remove the now useless `if (children != null)` calls. Wdyt?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zookeeper pull request #160: ZOOKEEPER-2680:Correct DataNode.getChildren() i...
Posted by edwardoliveira <gi...@git.apache.org>.
Github user edwardoliveira commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/160#discussion_r98967876
--- Diff: src/java/main/org/apache/zookeeper/server/DataTree.java ---
@@ -1045,18 +1046,16 @@ private void getCounts(String path, Counts counts) {
if (node == null) {
return;
}
- Set<String> children = null;
+ String[] children = null;
int len = 0;
synchronized (node) {
- children = node.getChildren();
+ Set<String> childs = node.getChildren();
+ children = childs.toArray(new String[childs.size()]);
--- End diff --
FYI (only): I *guess* some tests broke 'cause `toArray` copies the contents of `childs` to `children` while `node.getChildren()` returns a internal reference (subject to underneath change) to `children`. So, it would need to be something like `children = new HashSet<>(childs);`
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zookeeper pull request #160: ZOOKEEPER-2680:Correct DataNode.getChildren() i...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/zookeeper/pull/160
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zookeeper issue #160: ZOOKEEPER-2680:Correct DataNode.getChildren() inconsis...
Posted by hanm <gi...@git.apache.org>.
Github user hanm commented on the issue:
https://github.com/apache/zookeeper/pull/160
+1, merging to master (the jira mentioned patch to 3.4 and 3.5 will be sent separately).
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zookeeper issue #160: ZOOKEEPER-2680:Correct DataNode.getChildren() inconsis...
Posted by eribeiro <gi...@git.apache.org>.
Github user eribeiro commented on the issue:
https://github.com/apache/zookeeper/pull/160
+1. LGTM. :+1:
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zookeeper pull request #160: ZOOKEEPER-2680:Correct DataNode.getChildren() i...
Posted by eribeiro <gi...@git.apache.org>.
Github user eribeiro commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/160#discussion_r98709271
--- Diff: src/java/test/org/apache/zookeeper/server/DataNodeTest.java ---
@@ -0,0 +1,54 @@
+/**
+ * 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.*;
--- End diff --
FIY, ZK codebase usually don't put the static import in `Assert` methods, so the methods below are like `Assert.assertNotNull()`
Of course, this is not consistent across the whole codebase and I myself prefer the static import, but usually the wildcard is frowned upon here, afaik.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zookeeper pull request #160: ZOOKEEPER-2680:Correct DataNode.getChildren() i...
Posted by eribeiro <gi...@git.apache.org>.
Github user eribeiro commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/160#discussion_r98973726
--- Diff: src/java/main/org/apache/zookeeper/server/DataTree.java ---
@@ -1045,18 +1046,16 @@ private void getCounts(String path, Counts counts) {
if (node == null) {
return;
}
- Set<String> children = null;
+ String[] children = null;
int len = 0;
synchronized (node) {
- children = node.getChildren();
+ Set<String> childs = node.getChildren();
--- End diff --
But as all tests are passing, that's fine. :smiley: Congrats!
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zookeeper issue #160: ZOOKEEPER-2680:Correct DataNode.getChildren() inconsis...
Posted by arshadmohammad <gi...@git.apache.org>.
Github user arshadmohammad commented on the issue:
https://github.com/apache/zookeeper/pull/160
checking the CI failure.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zookeeper pull request #160: ZOOKEEPER-2680:Correct DataNode.getChildren() i...
Posted by eribeiro <gi...@git.apache.org>.
Github user eribeiro commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/160#discussion_r98745482
--- Diff: src/java/test/org/apache/zookeeper/server/DataNodeTest.java ---
@@ -0,0 +1,54 @@
+/**
+ * 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.*;
--- End diff --
Fair enough.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zookeeper pull request #160: ZOOKEEPER-2680:Correct DataNode.getChildren() i...
Posted by eribeiro <gi...@git.apache.org>.
Github user eribeiro commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/160#discussion_r98662079
--- Diff: src/java/test/org/apache/zookeeper/server/DataNodeTest.java ---
@@ -0,0 +1,54 @@
+/**
+ * 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
--- End diff --
I usually would agree with separating in two methods, but I think we can leave those in same method as they are so closely related... Aestetically pleasant, I mean. But just my opinion. :smile:
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zookeeper pull request #160: ZOOKEEPER-2680:Correct DataNode.getChildren() i...
Posted by arshadmohammad <gi...@git.apache.org>.
Github user arshadmohammad commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/160#discussion_r98743966
--- Diff: src/java/test/org/apache/zookeeper/server/DataNodeTest.java ---
@@ -0,0 +1,54 @@
+/**
+ * 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
--- End diff --
Done
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zookeeper pull request #160: ZOOKEEPER-2680:Correct DataNode.getChildren() i...
Posted by eribeiro <gi...@git.apache.org>.
Github user eribeiro commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/160#discussion_r98745143
--- Diff: src/java/test/org/apache/zookeeper/server/DataNodeTest.java ---
@@ -0,0 +1,66 @@
+/**
+ * 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 testGetChildrenRetrunsImmutableEmptySet() {
--- End diff --
Typo: retruns
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zookeeper pull request #160: ZOOKEEPER-2680:Correct DataNode.getChildren() i...
Posted by eribeiro <gi...@git.apache.org>.
Github user eribeiro commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/160#discussion_r98745304
--- Diff: src/java/test/org/apache/zookeeper/server/DataNodeTest.java ---
@@ -0,0 +1,66 @@
+/**
+ * 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 testGetChildrenRetrunsImmutableEmptySet() {
+ // Returned empty set must not be modifiable
--- End diff --
IMO, the method name makes this comment unnecessary
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zookeeper pull request #160: ZOOKEEPER-2680:Correct DataNode.getChildren() i...
Posted by eribeiro <gi...@git.apache.org>.
Github user eribeiro commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/160#discussion_r98968169
--- Diff: src/java/main/org/apache/zookeeper/server/DataTree.java ---
@@ -1045,18 +1046,16 @@ private void getCounts(String path, Counts counts) {
if (node == null) {
return;
}
- Set<String> children = null;
+ String[] children = null;
int len = 0;
synchronized (node) {
- children = node.getChildren();
+ Set<String> childs = node.getChildren();
--- End diff --
@arshadmohammad FYI (only): I guess some tests broke 'cause toArray copies the contents of childs to children while node.getChildren() returns a internal reference (subject to underneath change) to children. So, it would need to be something like children = new HashSet<>(childs);
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zookeeper pull request #160: ZOOKEEPER-2680:Correct DataNode.getChildren() i...
Posted by eribeiro <gi...@git.apache.org>.
Github user eribeiro commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/160#discussion_r98710030
--- Diff: src/java/test/org/apache/zookeeper/server/DataNodeTest.java ---
@@ -0,0 +1,54 @@
+/**
+ * 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
--- End diff --
Ignore my previous comment. +1 about putting this in a separate method.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zookeeper pull request #160: ZOOKEEPER-2680:Correct DataNode.getChildren() i...
Posted by arshadmohammad <gi...@git.apache.org>.
Github user arshadmohammad commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/160#discussion_r98743925
--- Diff: src/java/test/org/apache/zookeeper/server/DataNodeTest.java ---
@@ -0,0 +1,54 @@
+/**
+ * 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.*;
--- End diff --
I think it is OK to have one import instead of multiple imports. Not changing any thing here.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zookeeper pull request #160: ZOOKEEPER-2680:Correct DataNode.getChildren() i...
Posted by afine <gi...@git.apache.org>.
Github user afine commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/160#discussion_r98578122
--- Diff: src/java/test/org/apache/zookeeper/server/DataNodeTest.java ---
@@ -0,0 +1,54 @@
+/**
+ * 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
--- End diff --
nit: make this a separate test
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zookeeper issue #160: ZOOKEEPER-2680:Correct DataNode.getChildren() inconsis...
Posted by arshadmohammad <gi...@git.apache.org>.
Github user arshadmohammad commented on the issue:
https://github.com/apache/zookeeper/pull/160
1. yes, changes should be applied to branch-3.4 and branch-3.5 also. I will raise merge request for branch-3.4 and branch-3.5 after it is committed to master
2. This is very much needed. Thanks :-). I removed Null check from all references of getChildren and corrected the code as per the need
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---