You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@zookeeper.apache.org by Reidddddd <gi...@git.apache.org> on 2018/09/29 09:56:24 UTC
[GitHub] zookeeper pull request #650: ZOOKEEPER-1908: setAcl should be have a recursi...
GitHub user Reidddddd opened a pull request:
https://github.com/apache/zookeeper/pull/650
ZOOKEEPER-1908: setAcl should be have a recursive function
Let setAcl support recursive option, no new UT added.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/Reidddddd/zookeeper ZOOKEEPER-1908
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/zookeeper/pull/650.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 #650
----
commit 9ec3d83707163feb9896c9a5d79d4ecfd224faa1
Author: Reid Chan <re...@...>
Date: 2018-09-29T09:48:50Z
ZOOKEEPER-1908: setAcl should be have a recursive function
----
---
[GitHub] zookeeper issue #650: ZOOKEEPER-1908: setAcl should be have a recursive func...
Posted by Reidddddd <gi...@git.apache.org>.
Github user Reidddddd commented on the issue:
https://github.com/apache/zookeeper/pull/650
- Add descriptions about SetAclCommand.class
- Fix the variable overriding.
- Fix missed @Test annotation
- Add a test case for unsetAcl znode.
- Fix the wrong order of assertion.
---
[GitHub] zookeeper pull request #650: ZOOKEEPER-1908: setAcl should be have a recursi...
Posted by anmolnar <gi...@git.apache.org>.
Github user anmolnar commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/650#discussion_r221899955
--- Diff: src/java/test/org/apache/zookeeper/ZooKeeperTest.java ---
@@ -566,4 +566,25 @@ public void testLsrNonexistantZnodeCommand() throws Exception {
Assert.assertEquals(KeeperException.Code.NONODE, ((KeeperException)e.getCause()).code());
}
}
+
+ public void testSetAclRecursive() throws Exception {
+ final ZooKeeper zk = createClient();
+ final byte[] EMPTY = new byte[0];
+
+ zk.setData("/", EMPTY, -1);
+ zk.create("/a", EMPTY, Ids.OPEN_ACL_UNSAFE, CreateMode.PERSISTENT);
+ zk.create("/a/b", EMPTY, Ids.OPEN_ACL_UNSAFE, CreateMode.PERSISTENT);
+ zk.create("/a/b/c", EMPTY, Ids.OPEN_ACL_UNSAFE, CreateMode.PERSISTENT);
+ zk.create("/a/d", EMPTY, Ids.OPEN_ACL_UNSAFE, CreateMode.PERSISTENT);
+
+ ZooKeeperMain zkMain = new ZooKeeperMain(zk);
+ String setAclCommand = "setAcl -R /a world:anyone:r";
+ zkMain.cl.parseCommand(setAclCommand);
+ Assert.assertFalse(zkMain.processZKCmd(zkMain.cl));
+
+ Assert.assertEquals(zk.getACL("/a", new Stat()), Ids.READ_ACL_UNSAFE);
--- End diff --
Parameter order should be the other way around, right?
Expected value first, the actual second.
A short description would also be useful.
---
[GitHub] zookeeper issue #650: ZOOKEEPER-1908: setAcl should be have a recursive func...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit commented on the issue:
https://github.com/apache/zookeeper/pull/650
Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/PreCommit-ZOOKEEPER-github-pr-build/2287/
---
[GitHub] zookeeper pull request #650: ZOOKEEPER-1908: setAcl should be have a recursi...
Posted by Reidddddd <gi...@git.apache.org>.
Github user Reidddddd commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/650#discussion_r221446442
--- Diff: src/java/main/org/apache/zookeeper/cli/SetAclCommand.java ---
@@ -35,10 +37,11 @@
{
options.addOption("s", false, "stats");
options.addOption("v", true, "version");
+ options.addOption("R", false, "recursive");
}
public SetAclCommand() {
- super("setAcl", "[-s] [-v version] path acl");
+ super("setAcl", "[-s] [-v version] [-R] path acl");
}
--- End diff --
And thanks for the code pointer.
---
[GitHub] zookeeper issue #650: ZOOKEEPER-1908: setAcl should be have a recursive func...
Posted by eolivelli <gi...@git.apache.org>.
Github user eolivelli commented on the issue:
https://github.com/apache/zookeeper/pull/650
@Reidddddd please change the description now, you have added a UT :)
---
[GitHub] zookeeper issue #650: ZOOKEEPER-1908: setAcl should be have a recursive func...
Posted by Reidddddd <gi...@git.apache.org>.
Github user Reidddddd commented on the issue:
https://github.com/apache/zookeeper/pull/650
I can add one if none exists.
---
[GitHub] zookeeper pull request #650: ZOOKEEPER-1908: setAcl should be have a recursi...
Posted by lvfangmin <gi...@git.apache.org>.
Github user lvfangmin commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/650#discussion_r221472342
--- Diff: src/java/main/org/apache/zookeeper/cli/SetAclCommand.java ---
@@ -69,9 +72,22 @@ public boolean exec() throws CliException {
version = -1;
}
try {
- Stat stat = zk.setACL(path, acl, version);
- if (cl.hasOption("s")) {
- new StatPrinter(out).print(stat);
+ if (cl.hasOption("R")) {
+ ZKUtil.visitSubTreeDFS(zk, path, false, new StringCallback() {
+ @Override
+ public void processResult(int rc, String path, Object ctx, String name) {
+ try {
+ zk.setACL(path, acl, version);
+ } catch (KeeperException | InterruptedException e) {
+ out.print(e.getMessage());
+ }
+ }
+ });
+ } else {
+ Stat stat = zk.setACL(path, acl, version);
+ if (cl.hasOption("s")) {
--- End diff --
Does that mean -s and -R is exclusive with watch other?
If yes, maybe we should add it to the description, otherwise we should implement this in -R as well.
---
[GitHub] zookeeper issue #650: ZOOKEEPER-1908: setAcl should be have a recursive func...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit commented on the issue:
https://github.com/apache/zookeeper/pull/650
Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/PreCommit-ZOOKEEPER-github-pr-build/2306/
---
[GitHub] zookeeper issue #650: ZOOKEEPER-1908: setAcl should be have a recursive func...
Posted by Reidddddd <gi...@git.apache.org>.
Github user Reidddddd commented on the issue:
https://github.com/apache/zookeeper/pull/650
Can you give a pointer where are those cli related tests?
---
[GitHub] zookeeper issue #650: ZOOKEEPER-1908: setAcl should be have a recursive func...
Posted by lvfangmin <gi...@git.apache.org>.
Github user lvfangmin commented on the issue:
https://github.com/apache/zookeeper/pull/650
retest this please
---
[GitHub] zookeeper pull request #650: ZOOKEEPER-1908: setAcl should be have a recursi...
Posted by Reidddddd <gi...@git.apache.org>.
Github user Reidddddd commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/650#discussion_r221446349
--- Diff: src/java/main/org/apache/zookeeper/cli/SetAclCommand.java ---
@@ -35,10 +37,11 @@
{
options.addOption("s", false, "stats");
options.addOption("v", true, "version");
+ options.addOption("R", false, "recursive");
}
public SetAclCommand() {
- super("setAcl", "[-s] [-v version] path acl");
+ super("setAcl", "[-s] [-v version] [-R] path acl");
}
--- End diff --
It is a parameter of option 'v', not description about what option 'v' is.
e.g. setAcl -v 5 /path/a ****, means set acl to specific znode's 5th version.
---
[GitHub] zookeeper pull request #650: ZOOKEEPER-1908: setAcl should be have a recursi...
Posted by maoling <gi...@git.apache.org>.
Github user maoling commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/650#discussion_r221445920
--- Diff: src/java/main/org/apache/zookeeper/cli/SetAclCommand.java ---
@@ -35,10 +37,11 @@
{
options.addOption("s", false, "stats");
options.addOption("v", true, "version");
+ options.addOption("R", false, "recursive");
}
public SetAclCommand() {
- super("setAcl", "[-s] [-v version] path acl");
+ super("setAcl", "[-s] [-v version] [-R] path acl");
}
--- End diff --
- Since there is no documentation about setAcl,so the more detailed, the better.
- "[-s stat] [-v version] [-R recursive] path acl"
---
[GitHub] zookeeper issue #650: ZOOKEEPER-1908: setAcl should be have a recursive func...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit commented on the issue:
https://github.com/apache/zookeeper/pull/650
Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/PreCommit-ZOOKEEPER-github-pr-build/2292/
---
[GitHub] zookeeper pull request #650: ZOOKEEPER-1908: setAcl should be have a recursi...
Posted by lvfangmin <gi...@git.apache.org>.
Github user lvfangmin commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/650#discussion_r221472284
--- Diff: src/java/main/org/apache/zookeeper/cli/SetAclCommand.java ---
@@ -69,9 +72,22 @@ public boolean exec() throws CliException {
version = -1;
}
try {
- Stat stat = zk.setACL(path, acl, version);
- if (cl.hasOption("s")) {
- new StatPrinter(out).print(stat);
+ if (cl.hasOption("R")) {
+ ZKUtil.visitSubTreeDFS(zk, path, false, new StringCallback() {
+ @Override
+ public void processResult(int rc, String path, Object ctx, String name) {
--- End diff --
It’s better to avoid overriding the path variable here.
---
[GitHub] zookeeper issue #650: ZOOKEEPER-1908: setAcl should be have a recursive func...
Posted by Reidddddd <gi...@git.apache.org>.
Github user Reidddddd commented on the issue:
https://github.com/apache/zookeeper/pull/650
Sure, do you mean creating another PR for branch-3.5?
---
[GitHub] zookeeper pull request #650: ZOOKEEPER-1908: setAcl should be have a recursi...
Posted by maoling <gi...@git.apache.org>.
Github user maoling commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/650#discussion_r221910075
--- Diff: src/java/test/org/apache/zookeeper/ZooKeeperTest.java ---
@@ -566,4 +566,25 @@ public void testLsrNonexistantZnodeCommand() throws Exception {
Assert.assertEquals(KeeperException.Code.NONODE, ((KeeperException)e.getCause()).code());
}
}
+
+ public void testSetAclRecursive() throws Exception {
+ final ZooKeeper zk = createClient();
--- End diff --
@Test?
---
[GitHub] zookeeper pull request #650: ZOOKEEPER-1908: setAcl should be have a recursi...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/zookeeper/pull/650
---
[GitHub] zookeeper issue #650: ZOOKEEPER-1908: setAcl should be have a recursive func...
Posted by Reidddddd <gi...@git.apache.org>.
Github user Reidddddd commented on the issue:
https://github.com/apache/zookeeper/pull/650
Failed UTs are unrelated, most of them are caused by timeout and oom.
---
[GitHub] zookeeper issue #650: ZOOKEEPER-1908: setAcl should be have a recursive func...
Posted by anmolnar <gi...@git.apache.org>.
Github user anmolnar commented on the issue:
https://github.com/apache/zookeeper/pull/650
Committed to master branch. Thanks @Reidddddd !
Unfortunately commit script has died in the middle, so I wasn't able to commit to 3.5
Would you please create a separate PR for that?
---
[GitHub] zookeeper issue #650: ZOOKEEPER-1908: setAcl should be have a recursive func...
Posted by eolivelli <gi...@git.apache.org>.
Github user eolivelli commented on the issue:
https://github.com/apache/zookeeper/pull/650
retest this please
---
[GitHub] zookeeper pull request #650: ZOOKEEPER-1908: setAcl should be have a recursi...
Posted by anmolnar <gi...@git.apache.org>.
Github user anmolnar commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/650#discussion_r221922741
--- Diff: src/java/main/org/apache/zookeeper/cli/SetAclCommand.java ---
@@ -69,9 +72,22 @@ public boolean exec() throws CliException {
version = -1;
}
try {
- Stat stat = zk.setACL(path, acl, version);
- if (cl.hasOption("s")) {
- new StatPrinter(out).print(stat);
+ if (cl.hasOption("R")) {
+ ZKUtil.visitSubTreeDFS(zk, path, false, new StringCallback() {
+ @Override
+ public void processResult(int rc, String path, Object ctx, String name) {
+ try {
+ zk.setACL(path, acl, version);
+ } catch (KeeperException | InterruptedException e) {
+ out.print(e.getMessage());
+ }
+ }
+ });
+ } else {
+ Stat stat = zk.setACL(path, acl, version);
+ if (cl.hasOption("s")) {
--- End diff --
Unfortunately we don't have documentation for the CLI. It would be useful to start one either on wiki, in docs or implement a more detailed help page, but I don't want to overload this PR.
What if we just add a more detailed javadoc to the class?
---
[GitHub] zookeeper pull request #650: ZOOKEEPER-1908: setAcl should be have a recursi...
Posted by tamaashu <gi...@git.apache.org>.
Github user tamaashu commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/650#discussion_r221430894
--- Diff: src/java/main/org/apache/zookeeper/cli/SetAclCommand.java ---
@@ -35,10 +37,11 @@
{
options.addOption("s", false, "stats");
options.addOption("v", true, "version");
+ options.addOption("R", false, "recursive");
}
public SetAclCommand() {
- super("setAcl", "[-s] [-v version] path acl");
+ super("setAcl", "[-s] [-v version] [-R] path acl");
}
--- End diff --
Since we don't have it for [-s] either, I think it's more consistent to keep [-R].
---
[GitHub] zookeeper pull request #650: ZOOKEEPER-1908: setAcl should be have a recursi...
Posted by maoling <gi...@git.apache.org>.
Github user maoling commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/650#discussion_r221446706
--- Diff: src/java/main/org/apache/zookeeper/cli/SetAclCommand.java ---
@@ -35,10 +37,11 @@
{
options.addOption("s", false, "stats");
options.addOption("v", true, "version");
+ options.addOption("R", false, "recursive");
}
public SetAclCommand() {
- super("setAcl", "[-s] [-v version] path acl");
+ super("setAcl", "[-s] [-v version] [-R] path acl");
}
--- End diff --
oops!I was wrong.
---
[GitHub] zookeeper issue #650: ZOOKEEPER-1908: setAcl should be have a recursive func...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit commented on the issue:
https://github.com/apache/zookeeper/pull/650
Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/PreCommit-ZOOKEEPER-github-pr-build/2331/
---
[GitHub] zookeeper pull request #650: ZOOKEEPER-1908: setAcl should be have a recursi...
Posted by maoling <gi...@git.apache.org>.
Github user maoling commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/650#discussion_r221430136
--- Diff: src/java/main/org/apache/zookeeper/cli/SetAclCommand.java ---
@@ -35,10 +37,11 @@
{
options.addOption("s", false, "stats");
options.addOption("v", true, "version");
+ options.addOption("R", false, "recursive");
}
public SetAclCommand() {
- super("setAcl", "[-s] [-v version] path acl");
+ super("setAcl", "[-s] [-v version] [-R] path acl");
}
--- End diff --
[-R recursive] is better?
---
[GitHub] zookeeper pull request #650: ZOOKEEPER-1908: setAcl should be have a recursi...
Posted by lvfangmin <gi...@git.apache.org>.
Github user lvfangmin commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/650#discussion_r222432633
--- Diff: src/java/main/org/apache/zookeeper/cli/SetAclCommand.java ---
@@ -19,12 +19,17 @@
import java.util.List;
import org.apache.commons.cli.*;
+import org.apache.zookeeper.AsyncCallback.StringCallback;
import org.apache.zookeeper.KeeperException;
+import org.apache.zookeeper.ZKUtil;
import org.apache.zookeeper.data.ACL;
import org.apache.zookeeper.data.Stat;
/**
- * setAcl command for cli
+ * setAcl command for cli.
+ * Available options are s for printing znode's stats, v for set version of znode(s), R for
+ * recursive setting. User can combine v and R options together, but not s and R considering the
+ * number of znodes could be large.
--- End diff --
Thanks for adding the description for this.
---
[GitHub] zookeeper issue #650: ZOOKEEPER-1908: setAcl should be have a recursive func...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit commented on the issue:
https://github.com/apache/zookeeper/pull/650
Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/PreCommit-ZOOKEEPER-github-pr-build/2291/
---
[GitHub] zookeeper issue #650: ZOOKEEPER-1908: setAcl should be have a recursive func...
Posted by anmolnar <gi...@git.apache.org>.
Github user anmolnar commented on the issue:
https://github.com/apache/zookeeper/pull/650
yep.
---