Grant Henke 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 8: (16 comments) http://gerrit.cloudera.org:8080/#/c/14202/8//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/14202/8//COMMIT_MSG@7 PS8, Line 7: [kudu-client] KUDU-2910 Add Singleton Class KuduClientCache nit: Can you break this into a title and description instead of a single sentence? http://gerrit.cloudera.org:8080/#/c/14202/8//COMMIT_MSG@8 PS8, Line 8: to kudu client module and can used across different Nit: Capitalize Kudu http://gerrit.cloudera.org:8080/#/c/14202/8//COMMIT_MSG@9 PS8, Line 9: integration like spark, Hive etc.. nit: Capitalize Spark http://gerrit.cloudera.org:8080/#/c/14202/7/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/7/java/kudu-client/src/main/java/org/apache/kudu/client/KuduClientCache.java@89 PS7, Line 89: public void clearCacheForTests() { Can you annotate this with: @InterfaceAudience.LimitedPrivate("Test") http://gerrit.cloudera.org:8080/#/c/14202/8/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/8/java/kudu-client/src/main/java/org/apache/kudu/client/KuduClientCache.java@38 PS8, Line 38: private final Map<String, ClientCacheValue> clientCache = Collections.synchronizedMap( Instead did you consider using a ConcurrentHashMap? I think that would avoid the need for explicit synchronization. Another option would be to use the Guava LoadingCache, that would allow you to use the CacheLoader to create new instances, the remove listener to close the clients, and use Cache.invalidateAll() Cache.cleanUp() for shutdown and test reset. If we didn't already depend on and use Guava I would avoid it, but given we already shade Guava it could be a good option. http://gerrit.cloudera.org:8080/#/c/14202/8/java/kudu-client/src/main/java/org/apache/kudu/client/KuduClientCache.java@89 PS8, Line 89: public void clearCacheForTests() { This should be synchronized if you are using the synchronized map. http://gerrit.cloudera.org:8080/#/c/14202/8/java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduClientCache.java File java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduClientCache.java: http://gerrit.cloudera.org:8080/#/c/14202/8/java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduClientCache.java@32 PS8, Line 32: private KuduClient client; Are client and asyncClient used? http://gerrit.cloudera.org:8080/#/c/14202/8/java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduClientCache.java@46 PS8, Line 46: * Test only if a single instance of KuduClientCache is launched across multiple threads. I am not sure you really need this test. The singleton pattern is common and the call to getInstance() is trivial. If you want a simple sanity check just calling getInstance() twice and making sure it's equal via reference equality is probably good enough. http://gerrit.cloudera.org:8080/#/c/14202/8/java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduClientCache.java@76 PS8, Line 76: @Test(timeout = 100000) This is a really long timeout given what the test is doing. Maybe drop a zero? http://gerrit.cloudera.org:8080/#/c/14202/8/java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduClientCache.java@79 PS8, Line 79: Mockito.when(kuduClientCache.getAsyncClient("localhost")).thenReturn(asyncClient); Do you need to mock? You should be able to get a real client by using the masters from the harness via `harness.getMasterAddressesAsString()`. http://gerrit.cloudera.org:8080/#/c/14202/8/java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduClientCache.java@84 PS8, Line 84: for(int i=0;i<THREADS;i++) { nit: spacing http://gerrit.cloudera.org:8080/#/c/14202/8/java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduClientCache.java@88 PS8, Line 88: asyncKuduClientList[l] = kuduClientCache.getAsyncClient("localhost"); See my comment about mocking above. http://gerrit.cloudera.org:8080/#/c/14202/8/java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduClientCache.java@91 PS8, Line 91: }.start(); I am not sure this is actually testing any sort of concurrency given the the threads are all started right away and will basically finish immediately. Instead you would need to create all the threads, have them wait on a latch, and then start them all at once. A description of that can be read here: https://github.com/junit-team/junit4/wiki/multithreaded-code-and-concurrency http://gerrit.cloudera.org:8080/#/c/14202/8/java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduClientCache.java@93 PS8, Line 93: try { Thread.sleep(10); } catch(InterruptedException ex) { } This is unnecessary. I will get called below in the while loop. http://gerrit.cloudera.org:8080/#/c/14202/8/java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduClientCache.java@95 PS8, Line 95: while(count.get() >= 1) { nit: > 0 http://gerrit.cloudera.org:8080/#/c/14202/8/java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduClientCache.java@96 PS8, Line 96: try { Thread.sleep(10); } catch(InterruptedException ex) { } Can you just let InterruptedException throw? You can add throws Exception to the test method any any "exceptional" behavior will fail the test. -- 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: 8 Gerrit-Owner: Sandish Kumar HN <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Grant Henke <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Sandish Kumar HN <[email protected]> Gerrit-Comment-Date: Wed, 11 Sep 2019 03:18:21 +0000 Gerrit-HasComments: Yes
