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 4: (6 comments) http://gerrit.cloudera.org:8080/#/c/14202/1/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/1/java/kudu-client/src/main/java/org/apache/kudu/client/KuduClientCache.java@41 PS1, Line 41: private KuduClientCache() {} > Adar, I'm making sure it stays as singleton class Not sure how my suggestion prevents this from being a singleton class. Instead of: private static KuduClientCache kuduClientCache = null; public static synchronized KuduClientCache getInstance() { if(kuduClientCache == null) { kuduClientCache = new KuduClientCache(); } return kuduClientCache; } Why can't we do: private static KuduClientCache kuduClientCache = new KuduClientCache(); public static KuduClientCache getInstance() { return kuduClientCache; } 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@38 PS4, Line 38: private final Map<String, ClientCacheValue> clientCache = Collections.synchronizedMap(new HashMap<>()); Too long; split and wrap. http://gerrit.cloudera.org:8080/#/c/14202/4/java/kudu-client/src/main/java/org/apache/kudu/client/KuduClientCache.java@82 PS4, Line 82: this. Nit: don't need this. http://gerrit.cloudera.org:8080/#/c/14202/4/java/kudu-client/src/main/java/org/apache/kudu/client/KuduClientCache.java@106 PS4, Line 106: class ClientCacheValue Could this be declared private? http://gerrit.cloudera.org:8080/#/c/14202/4/java/kudu-client/src/main/java/org/apache/kudu/client/KuduClientCache.java@107 PS4, Line 107: { Nit: should go on the previous line. http://gerrit.cloudera.org:8080/#/c/14202/4/java/kudu-client/src/main/java/org/apache/kudu/client/KuduClientCache.java@108 PS4, Line 108: public AsyncKuduClient asyncKuduClient; : public Thread runnable; Could be declared final. -- 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: 4 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 17:54:15 +0000 Gerrit-HasComments: Yes
