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 2020/11/06 07:40:48 UTC
[GitHub] [zookeeper] nsnhuang opened a new pull request #1525: Zookeeper 3994：Disconnect reason is wrong
nsnhuang opened a new pull request #1525:
URL: https://github.com/apache/zookeeper/pull/1525
----------------------------------------------------------------
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
[GitHub] [zookeeper] nsnhuang commented on pull request #1525: ZOOKEEPER-3994：Disconnect reason is wrong for NOT_READ_ONLY_CLIENT and CLIENT_ZXID_AHEAD
Posted by GitBox <gi...@apache.org>.
nsnhuang commented on pull request #1525:
URL: https://github.com/apache/zookeeper/pull/1525#issuecomment-723887775
> Impressive efforts, @nsnhuang! The build is failing because of stylistic issues, but apart from that, your contribution looks good to me.
That excites me！Thank you for your help.
Next, what should I do to solve the stylistic issues ?
----------------------------------------------------------------
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
[GitHub] [zookeeper] nsnhuang commented on pull request #1525: ZOOKEEPER-3994：Disconnect reason is wrong for NOT_READ_ONLY_CLIENT and CLIENT_ZXID_AHEAD
Posted by GitBox <gi...@apache.org>.
nsnhuang commented on pull request #1525:
URL: https://github.com/apache/zookeeper/pull/1525#issuecomment-728157480
> If you tell me your Jira id, I can add you to contributors list and assign Jiras yourself.
That excites me！I will keep trying. Here's my Jira information.
Username: nsnhuang
Full name: huangwenbo
----------------------------------------------------------------
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
[GitHub] [zookeeper] ctubbsii commented on a change in pull request #1525: ZOOKEEPER-3994：Disconnect reason is wrong for NOT_READ_ONLY_CLIENT and CLIENT_ZXID_AHEAD
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on a change in pull request #1525:
URL: https://github.com/apache/zookeeper/pull/1525#discussion_r523111518
##########
File path: zookeeper-server/src/test/java/org/apache/zookeeper/server/ZooKeeperServerTest.java
##########
@@ -135,4 +138,32 @@ public void testInvalidSnapshot() {
}
}
+ @Test
+ public void testClientZxidAhead() {
+ ZooKeeperServer zooKeeperServer = new ZooKeeperServer();
+ final ZKDatabase zkDatabase = new ZKDatabase(mock(FileTxnSnapLog.class));
+ zooKeeperServer.setZKDatabase(zkDatabase);
+
+ final ByteBuffer output = ByteBuffer.allocate(30);
+ // serialize a connReq
+ output.putInt(1);
+ // lastZxid
+ output.putLong(99L);
+ output.putInt(500);
+ output.putLong(123L);
+ output.putInt(1);
+ output.put((byte) 1);
+ output.put((byte) 1);
+ output.flip();
+
+ try {
+ final NIOServerCnxn nioServerCnxn = mock(NIOServerCnxn.class);
+ zooKeeperServer.processConnectRequest(nioServerCnxn, output);
+ } catch (Exception e) {
+ // expect
+ assertTrue(e instanceof NIOServerCnxn.CloseRequestException);
+ assertEquals(((NIOServerCnxn.CloseRequestException) e).getReason(), ServerCnxn.DisconnectReason.CLIENT_ZXID_AHEAD);
+ }
Review comment:
I believe there's an `assertThrows()` method that might be cleaner than try-catch, and which returns the exception thrown, so you can check additional state, like the `getReason()`. The try-catch syntax is fine, though.
----------------------------------------------------------------
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
[GitHub] [zookeeper] eolivelli commented on a change in pull request #1525: ZOOKEEPER-3994：Disconnect reason is wrong for NOT_READ_ONLY_CLIENT and CLIENT_ZXID_AHEAD
Posted by GitBox <gi...@apache.org>.
eolivelli commented on a change in pull request #1525:
URL: https://github.com/apache/zookeeper/pull/1525#discussion_r521276106
##########
File path: zookeeper-server/src/test/java/org/apache/zookeeper/server/ZooKeeperServerTest.java
##########
@@ -135,4 +138,32 @@ public void testInvalidSnapshot() {
}
}
+ @Test
+ public void testClientZxidAhead() {
+ ZooKeeperServer zooKeeperServer = new ZooKeeperServer();
+ final ZKDatabase zkDatabase = new ZKDatabase(mock(FileTxnSnapLog.class));
+ zooKeeperServer.setZKDatabase(zkDatabase);
+
+ final ByteBuffer output = ByteBuffer.allocate(30);
+ // serialize a connReq
+ output.putInt(1);
+ // lastZxid
+ output.putLong(99L);
+ output.putInt(500);
+ output.putLong(123L);
+ output.putInt(1);
+ output.put((byte) 1);
+ output.put((byte) 1);
+ output.flip();
+
+ try {
+ final NIOServerCnxn nioServerCnxn = mock(NIOServerCnxn.class);
+ zooKeeperServer.processConnectRequest(nioServerCnxn, output);
+ } catch (Exception e) {
+ // expect
+ assertTrue(TestServerCnxn.instanceofCloseRequestException(e));
Review comment:
As a tradeoff we can use e.getClass().getName() and make the assertion against the string
this why we don't have to add that trick and also we do not have to add a new public class
@anmolnar would it work for you ?
----------------------------------------------------------------
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
[GitHub] [zookeeper] anmolnar commented on pull request #1525: ZOOKEEPER-3994：Disconnect reason is wrong for NOT_READ_ONLY_CLIENT and CLIENT_ZXID_AHEAD
Posted by GitBox <gi...@apache.org>.
anmolnar commented on pull request #1525:
URL: https://github.com/apache/zookeeper/pull/1525#issuecomment-728141464
If you tell me your Jira id, I can add you to contributors list and assign Jiras yourself.
----------------------------------------------------------------
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
[GitHub] [zookeeper] nsnhuang commented on a change in pull request #1525: ZOOKEEPER-3994：Disconnect reason is wrong for NOT_READ_ONLY_CLIENT and CLIENT_ZXID_AHEAD
Posted by GitBox <gi...@apache.org>.
nsnhuang commented on a change in pull request #1525:
URL: https://github.com/apache/zookeeper/pull/1525#discussion_r522915868
##########
File path: zookeeper-server/src/test/java/org/apache/zookeeper/server/ZooKeeperServerTest.java
##########
@@ -135,4 +138,32 @@ public void testInvalidSnapshot() {
}
}
+ @Test
+ public void testClientZxidAhead() {
+ ZooKeeperServer zooKeeperServer = new ZooKeeperServer();
+ final ZKDatabase zkDatabase = new ZKDatabase(mock(FileTxnSnapLog.class));
+ zooKeeperServer.setZKDatabase(zkDatabase);
+
+ final ByteBuffer output = ByteBuffer.allocate(30);
+ // serialize a connReq
+ output.putInt(1);
+ // lastZxid
+ output.putLong(99L);
+ output.putInt(500);
+ output.putLong(123L);
+ output.putInt(1);
+ output.put((byte) 1);
+ output.put((byte) 1);
+ output.flip();
+
+ try {
+ final NIOServerCnxn nioServerCnxn = mock(NIOServerCnxn.class);
+ zooKeeperServer.processConnectRequest(nioServerCnxn, output);
+ } catch (Exception e) {
+ // expect
+ assertTrue(TestServerCnxn.instanceofCloseRequestException(e));
Review comment:
> If it's necessary for testing, feel free to make it public and annotate with `VisibleForTesting`. We do this every so often, it's neater than adding testing classes all over the place. Thoughts?
@anmolnar I have a question, Which package is' VisibleForTesting '?
----------------------------------------------------------------
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
[GitHub] [zookeeper] nsnhuang commented on a change in pull request #1525: ZOOKEEPER-3994：Disconnect reason is wrong for NOT_READ_ONLY_CLIENT and CLIENT_ZXID_AHEAD
Posted by GitBox <gi...@apache.org>.
nsnhuang commented on a change in pull request #1525:
URL: https://github.com/apache/zookeeper/pull/1525#discussion_r521855112
##########
File path: zookeeper-server/src/test/java/org/apache/zookeeper/server/ZooKeeperServerTest.java
##########
@@ -135,4 +138,32 @@ public void testInvalidSnapshot() {
}
}
+ @Test
+ public void testClientZxidAhead() {
+ ZooKeeperServer zooKeeperServer = new ZooKeeperServer();
+ final ZKDatabase zkDatabase = new ZKDatabase(mock(FileTxnSnapLog.class));
+ zooKeeperServer.setZKDatabase(zkDatabase);
+
+ final ByteBuffer output = ByteBuffer.allocate(30);
+ // serialize a connReq
+ output.putInt(1);
+ // lastZxid
+ output.putLong(99L);
+ output.putInt(500);
+ output.putLong(123L);
+ output.putInt(1);
+ output.put((byte) 1);
+ output.put((byte) 1);
+ output.flip();
+
+ try {
+ final NIOServerCnxn nioServerCnxn = mock(NIOServerCnxn.class);
+ zooKeeperServer.processConnectRequest(nioServerCnxn, output);
+ } catch (Exception e) {
+ // expect
+ assertTrue(TestServerCnxn.instanceofCloseRequestException(e));
Review comment:
> As a tradeoff we can use e.getClass().getName() and make the assertion against the string
> this why we don't have to add that trick and also we do not have to add a new public class
>
> @eolivelli It works for "e instanceof NIOServerCnxn.CloseRequestException", but can't know which disconnect reason is.
----------------------------------------------------------------
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
[GitHub] [zookeeper] ctubbsii commented on a change in pull request #1525: ZOOKEEPER-3994：Disconnect reason is wrong for NOT_READ_ONLY_CLIENT and CLIENT_ZXID_AHEAD
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on a change in pull request #1525:
URL: https://github.com/apache/zookeeper/pull/1525#discussion_r520687393
##########
File path: zookeeper-server/src/test/java/org/apache/zookeeper/server/ZooKeeperServerTest.java
##########
@@ -135,4 +138,32 @@ public void testInvalidSnapshot() {
}
}
+ @Test
+ public void testClientZxidAhead() {
+ ZooKeeperServer zooKeeperServer = new ZooKeeperServer();
+ final ZKDatabase zkDatabase = new ZKDatabase(mock(FileTxnSnapLog.class));
+ zooKeeperServer.setZKDatabase(zkDatabase);
+
+ final ByteBuffer output = ByteBuffer.allocate(30);
+ // serialize a connReq
+ output.putInt(1);
+ // lastZxid
+ output.putLong(99L);
+ output.putInt(500);
+ output.putLong(123L);
+ output.putInt(1);
+ output.put((byte) 1);
+ output.put((byte) 1);
+ output.flip();
+
+ try {
+ final NIOServerCnxn nioServerCnxn = mock(NIOServerCnxn.class);
+ zooKeeperServer.processConnectRequest(nioServerCnxn, output);
+ } catch (Exception e) {
+ // expect
+ assertTrue(TestServerCnxn.instanceofCloseRequestException(e));
Review comment:
> If it's necessary for testing, feel free to make it public and annotate with `VisibleForTesting`. We do this every so often, it's neater than adding testing classes all over the place. Thoughts?
@anmolnar Subclassing for testing is a valid strategy for testing that requires access to protected methods. The `VisibleForTesting` annotation isn't a great mitigation for unintentional API leakage and misuse, since the compiler won't catch improper uses of it, and neither will most IDEs. Also, it adds an additional dependency on wherever you're getting that annotation imported from (presumably Guava), when you might otherwise not need that dependency. I would encourage the use of subclassing to test protected methods, if it's simple enough to do so (as long as it's reasonable to put the subclass as an inner class inside the test itself). I would resort to increasing the visibility to public, only as a last resort. It is particularly important in ZooKeeper, to limit the public visibility on APIs, because of the fact that ZooKeeper doesn't declare specific packages or classes as "public API", so users of ZooKeeper must rely on the visibility and other factors to infer stability, a
nd they may not notice an annotation that does not trigger any compiler or IDE warning, and incorrectly infer that it is stable when it isn't.
----------------------------------------------------------------
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
[GitHub] [zookeeper] eolivelli commented on a change in pull request #1525: ZOOKEEPER-3994：Disconnect reason is wrong for NOT_READ_ONLY_CLIENT and CLIENT_ZXID_AHEAD
Posted by GitBox <gi...@apache.org>.
eolivelli commented on a change in pull request #1525:
URL: https://github.com/apache/zookeeper/pull/1525#discussion_r523165078
##########
File path: zookeeper-server/src/test/java/org/apache/zookeeper/server/ZooKeeperServerTest.java
##########
@@ -135,4 +138,32 @@ public void testInvalidSnapshot() {
}
}
+ @Test
+ public void testClientZxidAhead() {
+ ZooKeeperServer zooKeeperServer = new ZooKeeperServer();
+ final ZKDatabase zkDatabase = new ZKDatabase(mock(FileTxnSnapLog.class));
+ zooKeeperServer.setZKDatabase(zkDatabase);
+
+ final ByteBuffer output = ByteBuffer.allocate(30);
+ // serialize a connReq
+ output.putInt(1);
+ // lastZxid
+ output.putLong(99L);
+ output.putInt(500);
+ output.putLong(123L);
+ output.putInt(1);
+ output.put((byte) 1);
+ output.put((byte) 1);
+ output.flip();
+
+ try {
+ final NIOServerCnxn nioServerCnxn = mock(NIOServerCnxn.class);
+ zooKeeperServer.processConnectRequest(nioServerCnxn, output);
+ } catch (Exception e) {
+ // expect
+ assertTrue(e instanceof NIOServerCnxn.CloseRequestException);
+ assertEquals(((NIOServerCnxn.CloseRequestException) e).getReason(), ServerCnxn.DisconnectReason.CLIENT_ZXID_AHEAD);
+ }
Review comment:
Btw is look good to me even in this form :)
----------------------------------------------------------------
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
[GitHub] [zookeeper] anmolnar commented on a change in pull request #1525: ZOOKEEPER-3994：Disconnect reason is wrong for NOT_READ_ONLY_CLIENT and CLIENT_ZXID_AHEAD
Posted by GitBox <gi...@apache.org>.
anmolnar commented on a change in pull request #1525:
URL: https://github.com/apache/zookeeper/pull/1525#discussion_r520462009
##########
File path: zookeeper-server/src/test/java/org/apache/zookeeper/server/ZooKeeperServerTest.java
##########
@@ -135,4 +138,32 @@ public void testInvalidSnapshot() {
}
}
+ @Test
+ public void testClientZxidAhead() {
+ ZooKeeperServer zooKeeperServer = new ZooKeeperServer();
+ final ZKDatabase zkDatabase = new ZKDatabase(mock(FileTxnSnapLog.class));
+ zooKeeperServer.setZKDatabase(zkDatabase);
+
+ final ByteBuffer output = ByteBuffer.allocate(30);
+ // serialize a connReq
+ output.putInt(1);
+ // lastZxid
+ output.putLong(99L);
+ output.putInt(500);
+ output.putLong(123L);
+ output.putInt(1);
+ output.put((byte) 1);
+ output.put((byte) 1);
+ output.flip();
+
+ try {
+ final NIOServerCnxn nioServerCnxn = mock(NIOServerCnxn.class);
+ zooKeeperServer.processConnectRequest(nioServerCnxn, output);
+ } catch (Exception e) {
+ // expect
+ assertTrue(TestServerCnxn.instanceofCloseRequestException(e));
Review comment:
If it's necessary for testing, feel free to make it public and annotate with `VisibleForTesting`. We do this every so often, it's neater than adding testing classes all over the place. Thoughts?
----------------------------------------------------------------
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
[GitHub] [zookeeper] anmolnar commented on pull request #1525: ZOOKEEPER-3994：Disconnect reason is wrong for NOT_READ_ONLY_CLIENT and CLIENT_ZXID_AHEAD
Posted by GitBox <gi...@apache.org>.
anmolnar commented on pull request #1525:
URL: https://github.com/apache/zookeeper/pull/1525#issuecomment-728014646
@nsnhuang Thanks for keep working on this. I know it's getting a bit of a pain.
I looked at your patch more closely and see the following:
- ZookeeperServerTest is in the package `o.a.zookeeper.server`, hence it can access CloseRequestException if it's protected,
- while ReadOnlyZooKeeperServerTest is in package `o.a.zookeeper.server.quorum` (same as ReadonlyServer), that causes the problem,
- `CloseRequestException` is thrown in public methods too (processConnectRequest), covered by throws IOException which is a bit odd. Why is it even in a nested class, if it's used outside too?
Given the above I don't see anything wrong with making the exception public. So, you're code is fine.
About VisibleForTesting annotation, I was completely wrong: we don't use it anywhere in our code, because we don't have Guava as a dependency. Instead we use it in comments like `// VisibleForTesting`. :)
Never mind, you don't need it.
----------------------------------------------------------------
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
[GitHub] [zookeeper] ztzg commented on pull request #1525: ZOOKEEPER-3994：Disconnect reason is wrong for NOT_READ_ONLY_CLIENT and CLIENT_ZXID_AHEAD
Posted by GitBox <gi...@apache.org>.
ztzg commented on pull request #1525:
URL: https://github.com/apache/zookeeper/pull/1525#issuecomment-724202711
> @ztzg are you okay with the patch now ?
Yes! (Besides, it was checkstyle which was complaining, not me :)
----------------------------------------------------------------
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
[GitHub] [zookeeper] asfgit closed pull request #1525: ZOOKEEPER-3994：Disconnect reason is wrong for NOT_READ_ONLY_CLIENT and CLIENT_ZXID_AHEAD
Posted by GitBox <gi...@apache.org>.
asfgit closed pull request #1525:
URL: https://github.com/apache/zookeeper/pull/1525
----------------------------------------------------------------
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
[GitHub] [zookeeper] eolivelli commented on pull request #1525: ZOOKEEPER-3994：Disconnect reason is wrong for NOT_READ_ONLY_CLIENT and CLIENT_ZXID_AHEAD
Posted by GitBox <gi...@apache.org>.
eolivelli commented on pull request #1525:
URL: https://github.com/apache/zookeeper/pull/1525#issuecomment-723992830
I will merge as soon as we have a green build
looking forward to new contributions @nsnhuang !
----------------------------------------------------------------
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
[GitHub] [zookeeper] anmolnar commented on a change in pull request #1525: ZOOKEEPER-3994：Disconnect reason is wrong for NOT_READ_ONLY_CLIENT and CLIENT_ZXID_AHEAD
Posted by GitBox <gi...@apache.org>.
anmolnar commented on a change in pull request #1525:
URL: https://github.com/apache/zookeeper/pull/1525#discussion_r520463563
##########
File path: zookeeper-server/src/test/java/org/apache/zookeeper/server/ZooKeeperServerTest.java
##########
@@ -135,4 +138,32 @@ public void testInvalidSnapshot() {
}
}
+ @Test
+ public void testClientZxidAhead() {
+ ZooKeeperServer zooKeeperServer = new ZooKeeperServer();
+ final ZKDatabase zkDatabase = new ZKDatabase(mock(FileTxnSnapLog.class));
+ zooKeeperServer.setZKDatabase(zkDatabase);
+
+ final ByteBuffer output = ByteBuffer.allocate(30);
+ // serialize a connReq
+ output.putInt(1);
+ // lastZxid
+ output.putLong(99L);
+ output.putInt(500);
+ output.putLong(123L);
+ output.putInt(1);
+ output.put((byte) 1);
+ output.put((byte) 1);
+ output.flip();
+
+ try {
+ final NIOServerCnxn nioServerCnxn = mock(NIOServerCnxn.class);
+ zooKeeperServer.processConnectRequest(nioServerCnxn, output);
+ } catch (Exception e) {
+ // expect
+ assertTrue(TestServerCnxn.instanceofCloseRequestException(e));
Review comment:
You could also extract the `CloseRequestException` class in a separate file. It doesn't have to be a nested class.
----------------------------------------------------------------
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
[GitHub] [zookeeper] anmolnar commented on a change in pull request #1525: ZOOKEEPER-3994：Disconnect reason is wrong for NOT_READ_ONLY_CLIENT and CLIENT_ZXID_AHEAD
Posted by GitBox <gi...@apache.org>.
anmolnar commented on a change in pull request #1525:
URL: https://github.com/apache/zookeeper/pull/1525#discussion_r521257220
##########
File path: zookeeper-server/src/test/java/org/apache/zookeeper/server/ZooKeeperServerTest.java
##########
@@ -135,4 +138,32 @@ public void testInvalidSnapshot() {
}
}
+ @Test
+ public void testClientZxidAhead() {
+ ZooKeeperServer zooKeeperServer = new ZooKeeperServer();
+ final ZKDatabase zkDatabase = new ZKDatabase(mock(FileTxnSnapLog.class));
+ zooKeeperServer.setZKDatabase(zkDatabase);
+
+ final ByteBuffer output = ByteBuffer.allocate(30);
+ // serialize a connReq
+ output.putInt(1);
+ // lastZxid
+ output.putLong(99L);
+ output.putInt(500);
+ output.putLong(123L);
+ output.putInt(1);
+ output.put((byte) 1);
+ output.put((byte) 1);
+ output.flip();
+
+ try {
+ final NIOServerCnxn nioServerCnxn = mock(NIOServerCnxn.class);
+ zooKeeperServer.processConnectRequest(nioServerCnxn, output);
+ } catch (Exception e) {
+ // expect
+ assertTrue(TestServerCnxn.instanceofCloseRequestException(e));
Review comment:
I don't agree with subclassing for testing is a good strategy. I saw people hundreds of times reusing each other's subclasses with some modification just because it's already present and convenient to add some things. The result is a complete mess in the testing code, so I highly discourage people using it. Especially introducing it in ZooKeeper at this point in time.
A more preferred way is mocking: that's essentially subclassing and produces much neater code for testing. Maybe not as convenient as adding a new subclass, but it's all about getting the habit of using mocking libraries.
`VisibleForTesting` annotation: I don't have much experience with that, but that must be a reason why projects of Hadoop are using it all over the place. There should be some mechanism on the client side to trigger a warning when somebody is trying to access such member. Either way, ZooKeeper code is heavily using it already, so there's not much harm using is here as well.
In this particular case I think the best would be to extract the exception in a separate file under the same package. I don't see value in nested exception classes anyway.
----------------------------------------------------------------
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
[GitHub] [zookeeper] eolivelli commented on pull request #1525: ZOOKEEPER-3994：Disconnect reason is wrong for NOT_READ_ONLY_CLIENT and CLIENT_ZXID_AHEAD
Posted by GitBox <gi...@apache.org>.
eolivelli commented on pull request #1525:
URL: https://github.com/apache/zookeeper/pull/1525#issuecomment-723992960
@ztzg are you okay with the patch now ?
----------------------------------------------------------------
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
[GitHub] [zookeeper] nsnhuang commented on pull request #1525: ZOOKEEPER-3994：Disconnect reason is wrong for NOT_READ_ONLY_CLIENT and CLIENT_ZXID_AHEAD
Posted by GitBox <gi...@apache.org>.
nsnhuang commented on pull request #1525:
URL: https://github.com/apache/zookeeper/pull/1525#issuecomment-723996638
> I will merge as soon as we have a green build
>
> looking forward to new contributions @nsnhuang !
I am very excited to contribute code for the first time. I will keep trying, thank you for your help.
----------------------------------------------------------------
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
[GitHub] [zookeeper] anmolnar commented on pull request #1525: ZOOKEEPER-3994：Disconnect reason is wrong for NOT_READ_ONLY_CLIENT and CLIENT_ZXID_AHEAD
Posted by GitBox <gi...@apache.org>.
anmolnar commented on pull request #1525:
URL: https://github.com/apache/zookeeper/pull/1525#issuecomment-728140496
Merged to master. Thanks @nsnhuang !
I tried to cherry-pick it to branch-3.6, but there was a conflict. If you need this patch on other branches, please create separate PR.
----------------------------------------------------------------
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
[GitHub] [zookeeper] ztzg commented on pull request #1525: ZOOKEEPER-3994：Disconnect reason is wrong for NOT_READ_ONLY_CLIENT and CLIENT_ZXID_AHEAD
Posted by GitBox <gi...@apache.org>.
ztzg commented on pull request #1525:
URL: https://github.com/apache/zookeeper/pull/1525#issuecomment-723911040
Looking at [the Jenkins log](https://ci-hadoop.apache.org/blue/organizations/jenkins/zookeeper-precommit-github-pr/detail/PR-1525/2/pipeline#step-35-log-1094
), I see some `checkstyle` errors. You should be able to reproduce locally by running something akin to:
mvn checkstyle:check -Pfull-build
You will then see a report for validations such as the following:
[ERROR] […]/ReadOnlyZooKeeperServerTest.java:3: Using the '.*' form of import should be avoided - org.apache.zookeeper.server.*. [AvoidStarImport]
[ERROR] […]/ReadOnlyZooKeeperServerTest.java:5: Extra separation in import group before 'org.apache.zookeeper.server.persistence.FileTxnSnapLog' [ImportOrder]
[ERROR] […]/ReadOnlyZooKeeperServerTest.java:8: Extra separation in import group before 'java.nio.ByteBuffer' [ImportOrder]
[…]
Looking at [the Travis log](https://travis-ci.org/github/apache/zookeeper/jobs/741862504), I see two file missing license headers:
[WARNING] Files with unapproved licenses:
/home/travis/build/apache/zookeeper/zookeeper-server/src/test/java/org/apache/zookeeper/server/TestServerCnxn.java
/home/travis/build/apache/zookeeper/zookeeper-server/src/test/java/org/apache/zookeeper/server/quorum/ReadOnlyZooKeeperServerTest.java
[ERROR] Failed to execute goal org.apache.rat:apache-rat-plugin:0.13:check (default-cli) on project zookeeper: Too many files with unapproved license: 2 See RAT report in: /home/travis/build/apache/zookeeper/zookeeper-server/target/rat.txt -> [Help 1]
I would just copy the Apache license banner from one of the existing files.
----------------------------------------------------------------
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
[GitHub] [zookeeper] nsnhuang commented on pull request #1525: ZOOKEEPER-3994：Disconnect reason is wrong for NOT_READ_ONLY_CLIENT and CLIENT_ZXID_AHEAD
Posted by GitBox <gi...@apache.org>.
nsnhuang commented on pull request #1525:
URL: https://github.com/apache/zookeeper/pull/1525#issuecomment-722936677
> can you please a similar test for CLIENT_ZXID_AHEAD ?
> then we are good to good
> other reviewers already approved the linked patch
Of course ^o^
----------------------------------------------------------------
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
[GitHub] [zookeeper] ctubbsii commented on a change in pull request #1525: ZOOKEEPER-3994：Disconnect reason is wrong for NOT_READ_ONLY_CLIENT and CLIENT_ZXID_AHEAD
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on a change in pull request #1525:
URL: https://github.com/apache/zookeeper/pull/1525#discussion_r521825196
##########
File path: zookeeper-server/src/test/java/org/apache/zookeeper/server/ZooKeeperServerTest.java
##########
@@ -135,4 +138,32 @@ public void testInvalidSnapshot() {
}
}
+ @Test
+ public void testClientZxidAhead() {
+ ZooKeeperServer zooKeeperServer = new ZooKeeperServer();
+ final ZKDatabase zkDatabase = new ZKDatabase(mock(FileTxnSnapLog.class));
+ zooKeeperServer.setZKDatabase(zkDatabase);
+
+ final ByteBuffer output = ByteBuffer.allocate(30);
+ // serialize a connReq
+ output.putInt(1);
+ // lastZxid
+ output.putLong(99L);
+ output.putInt(500);
+ output.putLong(123L);
+ output.putInt(1);
+ output.put((byte) 1);
+ output.put((byte) 1);
+ output.flip();
+
+ try {
+ final NIOServerCnxn nioServerCnxn = mock(NIOServerCnxn.class);
+ zooKeeperServer.processConnectRequest(nioServerCnxn, output);
+ } catch (Exception e) {
+ // expect
+ assertTrue(TestServerCnxn.instanceofCloseRequestException(e));
Review comment:
> I don't agree with subclassing for testing is a good strategy.
> A more preferred way is mocking
To be clear, I don't think it's necessarily a *good* strategy, just that in *some* circumstances it might be an appropriate way to test something that you don't otherwise want accessible with higher visibility. I agree mocking is better, whenever possible.
> `VisibleForTesting` annotation: I don't have much experience with that, but that must be a reason why projects of Hadoop are using it all over the place.
I would caution against the idea that popularity is a good measure of reason. :wink:
My 2 cents already having been provided, I don't see any serious problems with any of the discussed strategies. They all seem to have some pros and cons. :smiley_cat:
----------------------------------------------------------------
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
[GitHub] [zookeeper] anmolnar commented on a change in pull request #1525: ZOOKEEPER-3994：Disconnect reason is wrong for NOT_READ_ONLY_CLIENT and CLIENT_ZXID_AHEAD
Posted by GitBox <gi...@apache.org>.
anmolnar commented on a change in pull request #1525:
URL: https://github.com/apache/zookeeper/pull/1525#discussion_r521281480
##########
File path: zookeeper-server/src/test/java/org/apache/zookeeper/server/ZooKeeperServerTest.java
##########
@@ -135,4 +138,32 @@ public void testInvalidSnapshot() {
}
}
+ @Test
+ public void testClientZxidAhead() {
+ ZooKeeperServer zooKeeperServer = new ZooKeeperServer();
+ final ZKDatabase zkDatabase = new ZKDatabase(mock(FileTxnSnapLog.class));
+ zooKeeperServer.setZKDatabase(zkDatabase);
+
+ final ByteBuffer output = ByteBuffer.allocate(30);
+ // serialize a connReq
+ output.putInt(1);
+ // lastZxid
+ output.putLong(99L);
+ output.putInt(500);
+ output.putLong(123L);
+ output.putInt(1);
+ output.put((byte) 1);
+ output.put((byte) 1);
+ output.flip();
+
+ try {
+ final NIOServerCnxn nioServerCnxn = mock(NIOServerCnxn.class);
+ zooKeeperServer.processConnectRequest(nioServerCnxn, output);
+ } catch (Exception e) {
+ // expect
+ assertTrue(TestServerCnxn.instanceofCloseRequestException(e));
Review comment:
@eolivelli Yep, sounds like a good compromise.
----------------------------------------------------------------
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
[GitHub] [zookeeper] anmolnar commented on a change in pull request #1525: ZOOKEEPER-3994：Disconnect reason is wrong for NOT_READ_ONLY_CLIENT and CLIENT_ZXID_AHEAD
Posted by GitBox <gi...@apache.org>.
anmolnar commented on a change in pull request #1525:
URL: https://github.com/apache/zookeeper/pull/1525#discussion_r520317429
##########
File path: zookeeper-server/src/test/java/org/apache/zookeeper/server/ZooKeeperServerTest.java
##########
@@ -135,4 +138,32 @@ public void testInvalidSnapshot() {
}
}
+ @Test
+ public void testClientZxidAhead() {
+ ZooKeeperServer zooKeeperServer = new ZooKeeperServer();
+ final ZKDatabase zkDatabase = new ZKDatabase(mock(FileTxnSnapLog.class));
+ zooKeeperServer.setZKDatabase(zkDatabase);
+
+ final ByteBuffer output = ByteBuffer.allocate(30);
+ // serialize a connReq
+ output.putInt(1);
+ // lastZxid
+ output.putLong(99L);
+ output.putInt(500);
+ output.putLong(123L);
+ output.putInt(1);
+ output.put((byte) 1);
+ output.put((byte) 1);
+ output.flip();
+
+ try {
+ final NIOServerCnxn nioServerCnxn = mock(NIOServerCnxn.class);
+ zooKeeperServer.processConnectRequest(nioServerCnxn, output);
+ } catch (Exception e) {
+ // expect
+ assertTrue(TestServerCnxn.instanceofCloseRequestException(e));
Review comment:
Why do you need a subclass to access these methods?
----------------------------------------------------------------
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
[GitHub] [zookeeper] nsnhuang commented on a change in pull request #1525: ZOOKEEPER-3994：Disconnect reason is wrong for NOT_READ_ONLY_CLIENT and CLIENT_ZXID_AHEAD
Posted by GitBox <gi...@apache.org>.
nsnhuang commented on a change in pull request #1525:
URL: https://github.com/apache/zookeeper/pull/1525#discussion_r521855112
##########
File path: zookeeper-server/src/test/java/org/apache/zookeeper/server/ZooKeeperServerTest.java
##########
@@ -135,4 +138,32 @@ public void testInvalidSnapshot() {
}
}
+ @Test
+ public void testClientZxidAhead() {
+ ZooKeeperServer zooKeeperServer = new ZooKeeperServer();
+ final ZKDatabase zkDatabase = new ZKDatabase(mock(FileTxnSnapLog.class));
+ zooKeeperServer.setZKDatabase(zkDatabase);
+
+ final ByteBuffer output = ByteBuffer.allocate(30);
+ // serialize a connReq
+ output.putInt(1);
+ // lastZxid
+ output.putLong(99L);
+ output.putInt(500);
+ output.putLong(123L);
+ output.putInt(1);
+ output.put((byte) 1);
+ output.put((byte) 1);
+ output.flip();
+
+ try {
+ final NIOServerCnxn nioServerCnxn = mock(NIOServerCnxn.class);
+ zooKeeperServer.processConnectRequest(nioServerCnxn, output);
+ } catch (Exception e) {
+ // expect
+ assertTrue(TestServerCnxn.instanceofCloseRequestException(e));
Review comment:
> As a tradeoff we can use e.getClass().getName() and make the assertion against the string
> this why we don't have to add that trick and also we do not have to add a new public class
>
> @eolivelli It works for "e instanceof NIOServerCnxn.CloseRequestException", but can't know what disconnect reason is.
----------------------------------------------------------------
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
[GitHub] [zookeeper] anmolnar commented on pull request #1525: ZOOKEEPER-3994：Disconnect reason is wrong for NOT_READ_ONLY_CLIENT and CLIENT_ZXID_AHEAD
Posted by GitBox <gi...@apache.org>.
anmolnar commented on pull request #1525:
URL: https://github.com/apache/zookeeper/pull/1525#issuecomment-728319213
It's done. Have fun! ;)
----------------------------------------------------------------
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
[GitHub] [zookeeper] nsnhuang commented on a change in pull request #1525: ZOOKEEPER-3994：Disconnect reason is wrong for NOT_READ_ONLY_CLIENT and CLIENT_ZXID_AHEAD
Posted by GitBox <gi...@apache.org>.
nsnhuang commented on a change in pull request #1525:
URL: https://github.com/apache/zookeeper/pull/1525#discussion_r523901393
##########
File path: zookeeper-server/src/test/java/org/apache/zookeeper/server/ZooKeeperServerTest.java
##########
@@ -135,4 +138,32 @@ public void testInvalidSnapshot() {
}
}
+ @Test
+ public void testClientZxidAhead() {
+ ZooKeeperServer zooKeeperServer = new ZooKeeperServer();
+ final ZKDatabase zkDatabase = new ZKDatabase(mock(FileTxnSnapLog.class));
+ zooKeeperServer.setZKDatabase(zkDatabase);
+
+ final ByteBuffer output = ByteBuffer.allocate(30);
+ // serialize a connReq
+ output.putInt(1);
+ // lastZxid
+ output.putLong(99L);
+ output.putInt(500);
+ output.putLong(123L);
+ output.putInt(1);
+ output.put((byte) 1);
+ output.put((byte) 1);
+ output.flip();
+
+ try {
+ final NIOServerCnxn nioServerCnxn = mock(NIOServerCnxn.class);
+ zooKeeperServer.processConnectRequest(nioServerCnxn, output);
+ } catch (Exception e) {
+ // expect
+ assertTrue(e instanceof NIOServerCnxn.CloseRequestException);
+ assertEquals(((NIOServerCnxn.CloseRequestException) e).getReason(), ServerCnxn.DisconnectReason.CLIENT_ZXID_AHEAD);
+ }
Review comment:
> I believe there's an `assertThrows()` method that might be cleaner than try-catch, and which returns the exception thrown, so you can check additional state, like the `getReason()`. The try-catch syntax is fine, though.
Thank you for your advice.
I have done as you suggested.
----------------------------------------------------------------
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
[GitHub] [zookeeper] nsnhuang commented on a change in pull request #1525: ZOOKEEPER-3994：Disconnect reason is wrong for NOT_READ_ONLY_CLIENT and CLIENT_ZXID_AHEAD
Posted by GitBox <gi...@apache.org>.
nsnhuang commented on a change in pull request #1525:
URL: https://github.com/apache/zookeeper/pull/1525#discussion_r520324273
##########
File path: zookeeper-server/src/test/java/org/apache/zookeeper/server/ZooKeeperServerTest.java
##########
@@ -135,4 +138,32 @@ public void testInvalidSnapshot() {
}
}
+ @Test
+ public void testClientZxidAhead() {
+ ZooKeeperServer zooKeeperServer = new ZooKeeperServer();
+ final ZKDatabase zkDatabase = new ZKDatabase(mock(FileTxnSnapLog.class));
+ zooKeeperServer.setZKDatabase(zkDatabase);
+
+ final ByteBuffer output = ByteBuffer.allocate(30);
+ // serialize a connReq
+ output.putInt(1);
+ // lastZxid
+ output.putLong(99L);
+ output.putInt(500);
+ output.putLong(123L);
+ output.putInt(1);
+ output.put((byte) 1);
+ output.put((byte) 1);
+ output.flip();
+
+ try {
+ final NIOServerCnxn nioServerCnxn = mock(NIOServerCnxn.class);
+ zooKeeperServer.processConnectRequest(nioServerCnxn, output);
+ } catch (Exception e) {
+ // expect
+ assertTrue(TestServerCnxn.instanceofCloseRequestException(e));
Review comment:
https://github.com/apache/zookeeper/pull/1518#issuecomment-722369988
It’s necessary to do that.
'org.apache.zookeeper.server.ServerCnxn.CloseRequestException' has protected access in 'org.apache.zookeeper.server.ServerCnxn', so I can't use it in ReadOnlyZooKeeperServerTest.java.
Do you have a better way to sovle 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
[GitHub] [zookeeper] eolivelli commented on pull request #1525: ZOOKEEPER-3994：Disconnect reason is wrong for NOT_READ_ONLY_CLIENT and CLIENT_ZXID_AHEAD
Posted by GitBox <gi...@apache.org>.
eolivelli commented on pull request #1525:
URL: https://github.com/apache/zookeeper/pull/1525#issuecomment-728022087
@anmolnar can you merge?
----------------------------------------------------------------
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
[GitHub] [zookeeper] ztzg edited a comment on pull request #1525: ZOOKEEPER-3994：Disconnect reason is wrong for NOT_READ_ONLY_CLIENT and CLIENT_ZXID_AHEAD
Posted by GitBox <gi...@apache.org>.
ztzg edited a comment on pull request #1525:
URL: https://github.com/apache/zookeeper/pull/1525#issuecomment-723911040
Looking at [the Jenkins log](https://ci-hadoop.apache.org/blue/organizations/jenkins/zookeeper-precommit-github-pr/detail/PR-1525/2/pipeline#step-35-log-1094
), I see some `checkstyle` errors. You should be able to reproduce locally by running something akin to:
mvn checkstyle:check -Pfull-build
You will then see a report for validations such as the following:
[ERROR] […]/ReadOnlyZooKeeperServerTest.java:3: Using the '.*' form of import should be avoided - org.apache.zookeeper.server.*. [AvoidStarImport]
[ERROR] […]/ReadOnlyZooKeeperServerTest.java:5: Extra separation in import group before 'org.apache.zookeeper.server.persistence.FileTxnSnapLog' [ImportOrder]
[ERROR] […]/ReadOnlyZooKeeperServerTest.java:8: Extra separation in import group before 'java.nio.ByteBuffer' [ImportOrder]
[…]
Looking at [the Travis log](https://travis-ci.org/github/apache/zookeeper/jobs/741862504), I see two file missing license headers:
[WARNING] Files with unapproved licenses:
/home/travis/build/apache/zookeeper/zookeeper-server/src/test/java/org/apache/zookeeper/server/TestServerCnxn.java
/home/travis/build/apache/zookeeper/zookeeper-server/src/test/java/org/apache/zookeeper/server/quorum/ReadOnlyZooKeeperServerTest.java
[ERROR] Failed to execute goal org.apache.rat:apache-rat-plugin:0.13:check (default-cli) on project zookeeper: Too many files with unapproved license: 2 See RAT report in: /home/travis/build/apache/zookeeper/zookeeper-server/target/rat.txt -> [Help 1]
I would just copy the Apache license banner from one of the existing files. As above, you can check locally with:
mvn apache-rat:check -Pfull-build
----------------------------------------------------------------
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
[GitHub] [zookeeper] nsnhuang commented on pull request #1525: ZOOKEEPER-3994：Disconnect reason is wrong for NOT_READ_ONLY_CLIENT and CLIENT_ZXID_AHEAD
Posted by GitBox <gi...@apache.org>.
nsnhuang commented on pull request #1525:
URL: https://github.com/apache/zookeeper/pull/1525#issuecomment-723047145
@eolivelli I've added test for CLIENT_ZXID_AHEAD
----------------------------------------------------------------
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