[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

[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