You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@zookeeper.apache.org by GitBox <gi...@apache.org> on 2019/12/18 15:41:00 UTC
[GitHub] [zookeeper] ravowlga123 opened a new pull request #1187:
ZOOKEEPER-3414 sync api throws NoNodeException when path is non-existent
ravowlga123 opened a new pull request #1187: ZOOKEEPER-3414 sync api throws NoNodeException when path is non-existent
URL: https://github.com/apache/zookeeper/pull/1187
As per ticket [ZOOKEEPER-3414](https://issues.apache.org/jira/browse/ZOOKEEPER-3414) while executing sync cmd NoNodeException should be thrown when path doesn't exist. To implement this I have used CliWrapperException and KeeperException.NoNodeException in the else condition of exec() present in SyncCommand.java
Please do let me know if the changes made make sense or if I have missed something.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [zookeeper] ravowlga123 commented on a change in pull request
#1187: ZOOKEEPER-3414 sync api throws NoNodeException when path is
non-existent
Posted by GitBox <gi...@apache.org>.
ravowlga123 commented on a change in pull request #1187: ZOOKEEPER-3414 sync api throws NoNodeException when path is non-existent
URL: https://github.com/apache/zookeeper/pull/1187#discussion_r362849825
##########
File path: zookeeper-server/src/main/java/org/apache/zookeeper/cli/SyncCommand.java
##########
@@ -74,7 +75,7 @@ public void processResult(int rc, String path, Object ctx) {
if (resultCode == 0) {
out.println("Sync is OK");
} else {
- out.println("Sync has failed. rc=" + resultCode);
+ throw new CliWrapperException(new KeeperException.NoNodeException(path));
}
Review comment:
@maoling I ended up using zookeeper.getData instead of exists in my latest commit and when path doesn't exist NoNode Exception is thrown which we can find in javadoc of getData so only change was to catch the exception and I have also changed the unit test accordingly.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [zookeeper] HorizonNet commented on a change in pull request #1187:
ZOOKEEPER-3414 sync api throws NoNodeException when path is non-existent
Posted by GitBox <gi...@apache.org>.
HorizonNet commented on a change in pull request #1187: ZOOKEEPER-3414 sync api throws NoNodeException when path is non-existent
URL: https://github.com/apache/zookeeper/pull/1187#discussion_r362977312
##########
File path: zookeeper-server/src/test/java/org/apache/zookeeper/ZooKeeperTest.java
##########
@@ -691,4 +691,19 @@ public void testSyncCommand() throws Exception {
runCommandExpect(cmd, expected);
}
+ @Test
+ public void testSyncCommandFailure() throws Exception {
+ final ZooKeeper zk = createClient();
+ SyncCommand cmd = new SyncCommand();
+ cmd.setZk(zk);
+ cmd.parse("sync /dddd".split(" "));
+ try {
+ runCommandExpect(cmd, new ArrayList<String>());
+ fail("Path doesn't exist so, command should fail.");
+ } catch (CliWrapperException e) {
+ assertEquals(KeeperException.Code.NONODE, ((KeeperException) e.getCause()).code());
+ assertEquals("Node does not exist: /dddd", e.getMessage());
+ }
+ }
+
Review comment:
Remove this empty line.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [zookeeper] maoling commented on a change in pull request #1187:
ZOOKEEPER-3414 sync api throws NoNodeException when path is non-existent
Posted by GitBox <gi...@apache.org>.
maoling commented on a change in pull request #1187: ZOOKEEPER-3414 sync api throws NoNodeException when path is non-existent
URL: https://github.com/apache/zookeeper/pull/1187#discussion_r362401303
##########
File path: zookeeper-server/src/main/java/org/apache/zookeeper/cli/SyncCommand.java
##########
@@ -74,7 +75,7 @@ public void processResult(int rc, String path, Object ctx) {
if (resultCode == 0) {
out.println("Sync is OK");
} else {
- out.println("Sync has failed. rc=" + resultCode);
+ throw new CliWrapperException(new KeeperException.NoNodeException(path));
}
Review comment:
Yes, it's what I thought
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [zookeeper] maoling commented on a change in pull request #1187:
ZOOKEEPER-3414 sync api throws NoNodeException when path is non-existent
Posted by GitBox <gi...@apache.org>.
maoling commented on a change in pull request #1187: ZOOKEEPER-3414 sync api throws NoNodeException when path is non-existent
URL: https://github.com/apache/zookeeper/pull/1187#discussion_r362401303
##########
File path: zookeeper-server/src/main/java/org/apache/zookeeper/cli/SyncCommand.java
##########
@@ -74,7 +75,7 @@ public void processResult(int rc, String path, Object ctx) {
if (resultCode == 0) {
out.println("Sync is OK");
} else {
- out.println("Sync has failed. rc=" + resultCode);
+ throw new CliWrapperException(new KeeperException.NoNodeException(path));
}
Review comment:
- Yes, it's what I thought
- We also need an unit case for this, look at an example from `ZooKeeperTest#testSyncCommand`
- `throw KeeperException.create(KeeperException.Code.get(resultCode));` is better?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [zookeeper] HorizonNet commented on a change in pull request #1187:
ZOOKEEPER-3414 sync api throws NoNodeException when path is non-existent
Posted by GitBox <gi...@apache.org>.
HorizonNet commented on a change in pull request #1187: ZOOKEEPER-3414 sync api throws NoNodeException when path is non-existent
URL: https://github.com/apache/zookeeper/pull/1187#discussion_r362977850
##########
File path: zookeeper-server/src/test/java/org/apache/zookeeper/ZooKeeperTest.java
##########
@@ -691,4 +691,19 @@ public void testSyncCommand() throws Exception {
runCommandExpect(cmd, expected);
}
+ @Test
+ public void testSyncCommandFailure() throws Exception {
+ final ZooKeeper zk = createClient();
+ SyncCommand cmd = new SyncCommand();
+ cmd.setZk(zk);
+ cmd.parse("sync /dddd".split(" "));
+ try {
+ runCommandExpect(cmd, new ArrayList<String>());
+ fail("Path doesn't exist so, command should fail.");
Review comment:
Better: `Command did not fail, even the path does not exist`
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [zookeeper] maoling commented on a change in pull request #1187:
ZOOKEEPER-3414 sync api throws NoNodeException when path is non-existent
Posted by GitBox <gi...@apache.org>.
maoling commented on a change in pull request #1187: ZOOKEEPER-3414 sync api throws NoNodeException when path is non-existent
URL: https://github.com/apache/zookeeper/pull/1187#discussion_r362401303
##########
File path: zookeeper-server/src/main/java/org/apache/zookeeper/cli/SyncCommand.java
##########
@@ -74,7 +75,7 @@ public void processResult(int rc, String path, Object ctx) {
if (resultCode == 0) {
out.println("Sync is OK");
} else {
- out.println("Sync has failed. rc=" + resultCode);
+ throw new CliWrapperException(new KeeperException.NoNodeException(path));
}
Review comment:
- Yes, it's what I thought
- We also need an unit case for this, look at an example from `ZooKeeperTest#testSyncCommand`
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [zookeeper] ravowlga123 edited a comment on issue #1187:
ZOOKEEPER-3414 sync api throws NoNodeException when path is non-existent
Posted by GitBox <gi...@apache.org>.
ravowlga123 edited a comment on issue #1187: ZOOKEEPER-3414 sync api throws NoNodeException when path is non-existent
URL: https://github.com/apache/zookeeper/pull/1187#issuecomment-587090038
Hi @maoling Thank You for the detailed explanation. I really appreciate it. I will update the changes shortly as per your explanation.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [zookeeper] ravowlga123 commented on a change in pull request
#1187: ZOOKEEPER-3414 sync api throws NoNodeException when path is
non-existent
Posted by GitBox <gi...@apache.org>.
ravowlga123 commented on a change in pull request #1187: ZOOKEEPER-3414 sync api throws NoNodeException when path is non-existent
URL: https://github.com/apache/zookeeper/pull/1187#discussion_r362512417
##########
File path: zookeeper-server/src/main/java/org/apache/zookeeper/cli/SyncCommand.java
##########
@@ -74,7 +75,7 @@ public void processResult(int rc, String path, Object ctx) {
if (resultCode == 0) {
out.println("Sync is OK");
} else {
- out.println("Sync has failed. rc=" + resultCode);
+ throw new CliWrapperException(new KeeperException.NoNodeException(path));
}
Review comment:
Ok @maoling I will use zookeeper.exists(path, false) after you get the result code and add a condition to check if stat!=null && resultcode == 0 then sync is ok then throw the exception.
Also will add a unit test for failure
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [zookeeper] maoling commented on issue #1187: ZOOKEEPER-3414 sync
api throws NoNodeException when path is non-existent
Posted by GitBox <gi...@apache.org>.
maoling commented on issue #1187: ZOOKEEPER-3414 sync api throws NoNodeException when path is non-existent
URL: https://github.com/apache/zookeeper/pull/1187#issuecomment-586071706
> I had the assumption of this should be implemented on server side. There's no guarantee of somebody not deleting the node after getData call.
- Yes, it is. Implementing on client side really has that side effect, especially a `delete` after `sync` by another client, just before `getData`.
- Overall, `sync + getData` isn't a good api design and some issues in it(e.g sync isn't a quorum operation and cannot get updated data when network partitioned, so I create [ZOOKEEPER-3600](https://issues.apache.org/jira/browse/ZOOKEEPER-3600) to depreciate it by a new Linearizability Read api.)
- A base summary of current `sync` implementation, when the client calls the `sync` api to a follower server, follower pends the `requestA`(in the `pendingSyncs`) and forwards `requestA` to leader. when leader has no outstanding proposals(`outstandingProposals`), leader will send a `SYNC` message to follower, follower will commit `requestA` util it receives the `SYNC` from leader.
- For the server side fix:
```
FinalRequestProcessor
case OpCode.sync: {
xxxxxxxxxxxxxxxxxxxxxxxxxx
path = syncRequest.getPath();
DataNode n = zks.getZKDatabase().getNode(path);
if (n == null) {
throw new KeeperException.NoNodeException();
}
rsp = new SyncResponse(path);
xxxxxxxxxxxxxxxxxxxxxxxxxx
}
SyncCommand:
} else if (resultCode == KeeperException.Code.NONODE.intValue()) {
throw new KeeperException.NoNodeException(path);
} else
```
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [zookeeper] ravowlga123 commented on issue #1187: ZOOKEEPER-3414
sync api throws NoNodeException when path is non-existent
Posted by GitBox <gi...@apache.org>.
ravowlga123 commented on issue #1187: ZOOKEEPER-3414 sync api throws NoNodeException when path is non-existent
URL: https://github.com/apache/zookeeper/pull/1187#issuecomment-587090038
Hi @maoling Thank You for the detailed explanation. I really appreciate it. I will update the changes shortly.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [zookeeper] anmolnar commented on issue #1187: ZOOKEEPER-3414 sync
api throws NoNodeException when path is non-existent
Posted by GitBox <gi...@apache.org>.
anmolnar commented on issue #1187: ZOOKEEPER-3414 sync api throws NoNodeException when path is non-existent
URL: https://github.com/apache/zookeeper/pull/1187#issuecomment-579761116
@ravowlga123 Issue has been reported my @maoling .
I had the assumption of this should be implemented on server side. There's no guarantee of somebody not deleting the node after getData call.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [zookeeper] asf-ci commented on issue #1187: ZOOKEEPER-3414 sync
api throws NoNodeException when path is non-existent
Posted by GitBox <gi...@apache.org>.
asf-ci commented on issue #1187: ZOOKEEPER-3414 sync api throws NoNodeException when path is non-existent
URL: https://github.com/apache/zookeeper/pull/1187#issuecomment-567927236
Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/PreCommit-ZOOKEEPER-github-pr-build-maven/1763/
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [zookeeper] ravowlga123 commented on a change in pull request
#1187: ZOOKEEPER-3414 sync api throws NoNodeException when path is
non-existent
Posted by GitBox <gi...@apache.org>.
ravowlga123 commented on a change in pull request #1187: ZOOKEEPER-3414 sync api throws NoNodeException when path is non-existent
URL: https://github.com/apache/zookeeper/pull/1187#discussion_r360351316
##########
File path: zookeeper-server/src/main/java/org/apache/zookeeper/cli/SyncCommand.java
##########
@@ -74,7 +75,7 @@ public void processResult(int rc, String path, Object ctx) {
if (resultCode == 0) {
out.println("Sync is OK");
} else {
- out.println("Sync has failed. rc=" + resultCode);
Review comment:
@eolivelli Sorry about that. I will add catch block for NoNodeException and commit the changes shortly.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [zookeeper] ravowlga123 commented on a change in pull request
#1187: ZOOKEEPER-3414 sync api throws NoNodeException when path is
non-existent
Posted by GitBox <gi...@apache.org>.
ravowlga123 commented on a change in pull request #1187: ZOOKEEPER-3414 sync api throws NoNodeException when path is non-existent
URL: https://github.com/apache/zookeeper/pull/1187#discussion_r362512417
##########
File path: zookeeper-server/src/main/java/org/apache/zookeeper/cli/SyncCommand.java
##########
@@ -74,7 +75,7 @@ public void processResult(int rc, String path, Object ctx) {
if (resultCode == 0) {
out.println("Sync is OK");
} else {
- out.println("Sync has failed. rc=" + resultCode);
+ throw new CliWrapperException(new KeeperException.NoNodeException(path));
}
Review comment:
Ok @maoling I will use zookeeper.exists(path, false) after you get the result code and add a condition to check if stat!=null && resultcode == 0 then sync is ok else throw the exception.
Also will add a unit test for failure
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [zookeeper] ravowlga123 commented on a change in pull request
#1187: ZOOKEEPER-3414 sync api throws NoNodeException when path is
non-existent
Posted by GitBox <gi...@apache.org>.
ravowlga123 commented on a change in pull request #1187: ZOOKEEPER-3414 sync api throws NoNodeException when path is non-existent
URL: https://github.com/apache/zookeeper/pull/1187#discussion_r362034417
##########
File path: zookeeper-server/src/main/java/org/apache/zookeeper/cli/SyncCommand.java
##########
@@ -74,7 +75,7 @@ public void processResult(int rc, String path, Object ctx) {
if (resultCode == 0) {
out.println("Sync is OK");
} else {
- out.println("Sync has failed. rc=" + resultCode);
+ throw new CliWrapperException(new KeeperException.NoNodeException(path));
}
Review comment:
Hi @maoling if I understand the alternative that you mentioned above correctly then I should use zookeeper.exists(path, false) after zookeeper.sync. Is this correct or have I misinterpreted your thought??
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [zookeeper] ravowlga123 commented on issue #1187: ZOOKEEPER-3414
sync api throws NoNodeException when path is non-existent
Posted by GitBox <gi...@apache.org>.
ravowlga123 commented on issue #1187: ZOOKEEPER-3414 sync api throws NoNodeException when path is non-existent
URL: https://github.com/apache/zookeeper/pull/1187#issuecomment-580838021
Hi @anmolnar if changes are not sufficient then I will look into zookeeper-server. Also would appreciate feedback from @maoling regarding this.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [zookeeper] maoling commented on a change in pull request #1187:
ZOOKEEPER-3414 sync api throws NoNodeException when path is non-existent
Posted by GitBox <gi...@apache.org>.
maoling commented on a change in pull request #1187: ZOOKEEPER-3414 sync api throws NoNodeException when path is non-existent
URL: https://github.com/apache/zookeeper/pull/1187#discussion_r361597100
##########
File path: zookeeper-server/src/main/java/org/apache/zookeeper/cli/SyncCommand.java
##########
@@ -74,7 +75,7 @@ public void processResult(int rc, String path, Object ctx) {
if (resultCode == 0) {
out.println("Sync is OK");
} else {
- out.println("Sync has failed. rc=" + resultCode);
+ throw new CliWrapperException(new KeeperException.NoNodeException(path));
}
Review comment:
- You may never get a `rc` code which tell you whether the path is exist because the server doesn't send it to the client
- One alternative way is `getData` or `exist` to check the existence of path after `sync`
- `sync` api has problems and not user-friendly, we will depreciate it in the further with a new linearity read api
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [zookeeper] HorizonNet commented on a change in pull request #1187:
ZOOKEEPER-3414 sync api throws NoNodeException when path is non-existent
Posted by GitBox <gi...@apache.org>.
HorizonNet commented on a change in pull request #1187: ZOOKEEPER-3414 sync api throws NoNodeException when path is non-existent
URL: https://github.com/apache/zookeeper/pull/1187#discussion_r362977411
##########
File path: zookeeper-server/src/test/java/org/apache/zookeeper/ZooKeeperTest.java
##########
@@ -691,4 +691,19 @@ public void testSyncCommand() throws Exception {
runCommandExpect(cmd, expected);
}
+ @Test
+ public void testSyncCommandFailure() throws Exception {
+ final ZooKeeper zk = createClient();
+ SyncCommand cmd = new SyncCommand();
Review comment:
This one can also be `final`.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services