Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/14202 )
Change subject: [kudu-client] KUDU-2910 Add KuduClientCache to kudu client module and can used across different integration like spark, Hive etc.. ...................................................................... Patch Set 1: (13 comments) http://gerrit.cloudera.org:8080/#/c/14202/1/java/kudu-client/build.gradle File java/kudu-client/build.gradle: http://gerrit.cloudera.org:8080/#/c/14202/1/java/kudu-client/build.gradle@35 PS1, Line 35: provided libs.hadoopCommon Do we really want to assume this in kudu-client, which currently has no Hadoop-specific dependencies outside of tests? Can we use a different shutdown hook implementation, such as the one built into Java? 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: PS1: Couple lines here are too long. Please wrap them. http://gerrit.cloudera.org:8080/#/c/14202/1/java/kudu-client/src/main/java/org/apache/kudu/client/KuduClientCache.java@38 PS1, Line 38: private Integer ShutdownHookPriority = 100; It's a static constant, right? How about: private static final int SHUTDOWN_HOOK_PRIORITY = 100; http://gerrit.cloudera.org:8080/#/c/14202/1/java/kudu-client/src/main/java/org/apache/kudu/client/KuduClientCache.java@40 PS1, Line 40: private final Map<String, Pair<AsyncKuduClient, Runnable>> clientCache = new HashMap<String, Pair<AsyncKuduClient, Runnable>>(); Should doc what's being cached here, what the keys mean, etc. 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 static KuduClientCache kuduClientCache = null; This should be moved ahead of the non-static members so it's more prominent. Also, couldn't you initialize it inline rather than in getInstance()? http://gerrit.cloudera.org:8080/#/c/14202/1/java/kudu-client/src/main/java/org/apache/kudu/client/KuduClientCache.java@46 PS1, Line 46: if(kuduClientCache == null) { Should be "if (" http://gerrit.cloudera.org:8080/#/c/14202/1/java/kudu-client/src/main/java/org/apache/kudu/client/KuduClientCache.java@56 PS1, Line 56: * @param kuduMaster Is this a comma-separated list of master addresses? Perhaps we could also offer a List<String> variant so users don't need to join the string themselves? http://gerrit.cloudera.org:8080/#/c/14202/1/java/kudu-client/src/main/java/org/apache/kudu/client/KuduClientCache.java@60 PS1, Line 60: synchronized (clientCache) { It's weird for most usage of the class to synchronize on the class instance and this member to synchronize on the map instead. http://gerrit.cloudera.org:8080/#/c/14202/1/java/kudu-client/src/main/java/org/apache/kudu/client/KuduClientCache.java@61 PS1, Line 61: kuduClientCache != null Why is this check necessary? The KuduCilentCache API prevents users from getting here without first calling getInstance(). http://gerrit.cloudera.org:8080/#/c/14202/1/java/kudu-client/src/main/java/org/apache/kudu/client/KuduClientCache.java@74 PS1, Line 74: Pair<AsyncKuduClient, Runnable> While this may have been acceptable Scala style, in Java we tend to discourage usage of Pairs and Tuples in favor of well-defined (and documented) classes. http://gerrit.cloudera.org:8080/#/c/14202/1/java/kudu-client/src/main/java/org/apache/kudu/client/KuduClientCache.java@81 PS1, Line 81: /** : * clears clientCache and make sure to close all the AsyncKuduClient instances. : * function is synchronized for thread safe. : */ Please reformat as per standard Javadoc style (one char between '*' and text, sentences start with capital letters, etc.) http://gerrit.cloudera.org:8080/#/c/14202/1/java/kudu-client/src/main/java/org/apache/kudu/client/KuduClientCache.java@87 PS1, Line 87: while (it.hasNext()) { Could this be an enhanced for loop? for (Pair<AsyncKuduClient, Runnable> cacheValue : clientCache.values()) { ... } http://gerrit.cloudera.org:8080/#/c/14202/1/java/kudu-client/src/main/java/org/apache/kudu/client/KuduClientCache.java@91 PS1, Line 91: exception Nit: 'e' or 'ex' would make it easier to distinguish between the type (Exception) and the value. -- 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: 1 Gerrit-Owner: Sandish Kumar HN <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Tue, 10 Sep 2019 00:45:47 +0000 Gerrit-HasComments: Yes
