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:
[email protected]
