[GitHub] [zookeeper] anmolnar commented on pull request #1525: ZOOKEEPER-3994：Disconnect reason is wrong for NOT_READ_ONLY_CLIENT and CLIENT_ZXID_AHEAD

[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