Adar Dembo has posted comments on this change.

Change subject: [java client] Extract ip2client out of AsyncKuduClient
......................................................................


Patch Set 1:

(3 comments)

I looked at the ConnectionCache API and members, but only skimmed the method 
implementations because I assumed that was just code motion. Was there any 
implementation change worth looking at?

http://gerrit.cloudera.org:8080/#/c/4717/1/java/kudu-client/src/main/java/org/apache/kudu/client/ConnectionCache.java
File java/kudu-client/src/main/java/org/apache/kudu/client/ConnectionCache.java:

Line 91:     this.kuduClient = client;
Isn't this a reference cycle between the AsyncKuduClient and ConnectionCache, 
which would prevent the two from being GC'ed? I think it would be cleaner if 
you passed in the facilities that ConnectionCache needs (e.g. the timer, the 
channel factory, etc.). Then you wouldn't need getters for those on 
AsyncKuduClient either.


PS1, Line 167: AsyncKuduClient
Odd to reference this class here, no?


Line 279:   static String getIP(final String host) {
Since this is static, I think it could remain in AsyncKuduClient, to keep the 
ConnectionCache API slimmer.


-- 
To view, visit http://gerrit.cloudera.org:8080/4717
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I48b0f3f262abd5ee26869202f661b3c25158f39c
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Jean-Daniel Cryans <jdcry...@apache.org>
Gerrit-Reviewer: Adar Dembo <a...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <d...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: Yes

Reply via email to