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