Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/14202 )
Change subject: [kudu-client] KUDU-2910 Add Singleton Class KuduClientCache to kudu client module and can used across different integration like spark, Hive etc.. ...................................................................... Patch Set 5: (4 comments) http://gerrit.cloudera.org:8080/#/c/14202/4/java/kudu-client/src/main/java/org/apache/kudu/client/KuduClientCache.java File java/kudu-client/src/main/java/org/apache/kudu/client/KuduClientCache.java: http://gerrit.cloudera.org:8080/#/c/14202/4/java/kudu-client/src/main/java/org/apache/kudu/client/KuduClientCache.java@82 PS4, Line 82: > Nit: don't need this. You missed this. http://gerrit.cloudera.org:8080/#/c/14202/5/java/kudu-client/src/main/java/org/apache/kudu/client/KuduClientCache.java File java/kudu-client/src/main/java/org/apache/kudu/client/KuduClientCache.java: http://gerrit.cloudera.org:8080/#/c/14202/5/java/kudu-client/src/main/java/org/apache/kudu/client/KuduClientCache.java@44 PS5, Line 44: public static synchronized KuduClientCache getInstance() { Doesn't need synchronization. http://gerrit.cloudera.org:8080/#/c/14202/5/java/kudu-client/src/main/java/org/apache/kudu/client/KuduClientCache.java@55 PS5, Line 55: public AsyncKuduClient getAsyncClient(String kuduMasters) { This needs to be synchronized; the synchronization on clientCache is insufficient because there are up to three distinct accesses. If you synchronized the entire method, the underlying map need not be synchronized. http://gerrit.cloudera.org:8080/#/c/14202/5/java/kudu-client/src/main/java/org/apache/kudu/client/KuduClientCache.java@72 PS5, Line 72: return clientCache.get(kuduMasters).asyncKuduClient; To avoid excessive nesting, put this case first so you can early return if it's true. Also, you can avoid the containsKey() by starting with a call to get() and either returning the value (if not null) or creating a new client (if null). -- To view, visit http://gerrit.cloudera.org:8080/14202 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I08f7bbd4f1f1223ac80175d4ab3eabe9c841ddf8 Gerrit-Change-Number: 14202 Gerrit-PatchSet: 5 Gerrit-Owner: Sandish Kumar HN <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Sandish Kumar HN <[email protected]> Gerrit-Comment-Date: Tue, 10 Sep 2019 20:57:27 +0000 Gerrit-HasComments: Yes
