kaisun2000 commented on a change in pull request #765: Add DedicatedZkClient and update DedicatedZkClientFactory URL: https://github.com/apache/helix/pull/765#discussion_r382290519
########## File path: zookeeper-api/src/test/java/org/apache/helix/zookeeper/impl/client/RealmAwareZkClientTestBase.java ########## @@ -0,0 +1,164 @@ +package org.apache.helix.zookeeper.impl.client; Review comment: As a general comment. As we took the approach to composition (aka create new class DedicatedZkClient), theoretically, it would be ideal to test the new method even it is a thin wrapper. If we choose the approach of modifying the ZKClient, since the code is not change, we can reason that limiting our testing to only the modified part, namely the checkingPath validity part would be enough. Considering composition is only a thin wrapper, I don't see the necessity to do it right now. However, later if we change the code of DedicatedZkClient implementation more, we should add more test then. ---------------------------------------------------------------- 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] With regards, Apache Git Services --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
