Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/12279 )
Change subject: KUDU-2543 pt 3 java: pass around authz tokens ...................................................................... Patch Set 2: (16 comments) http://gerrit.cloudera.org:8080/#/c/12279/2/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java File java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java: http://gerrit.cloudera.org:8080/#/c/12279/2/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java@752 PS2, Line 752: this.masterTable, tableId, tableId != null ? null : tableName, false); Nit: annotate what 'false' means? http://gerrit.cloudera.org:8080/#/c/12279/2/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java@1937 PS2, Line 1937: tokenReacquirer.handleAuthnTokenExpiration(rpc); Looks like some plumbing would net us exceptions here in the same way as handleInvalidAuthzToken. Is that something you're interesting in fixing? http://gerrit.cloudera.org:8080/#/c/12279/2/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java@1952 PS2, Line 1952: This may send an RPC to an : * external authorization metadata service, or retrieve it from a cache. authzTokenCache.get() never sends an RPC though. http://gerrit.cloudera.org:8080/#/c/12279/2/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduScanner.java File java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduScanner.java: http://gerrit.cloudera.org:8080/#/c/12279/2/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduScanner.java@920 PS2, Line 920: boolean needsAuthzToken() { return true; } Nit: break out. Elsewhere too. http://gerrit.cloudera.org:8080/#/c/12279/2/java/kudu-client/src/main/java/org/apache/kudu/client/AuthzTokenCache.java File java/kudu-client/src/main/java/org/apache/kudu/client/AuthzTokenCache.java: http://gerrit.cloudera.org:8080/#/c/12279/2/java/kudu-client/src/main/java/org/apache/kudu/client/AuthzTokenCache.java@59 PS2, Line 59: Pair<KuduRpc<?>, KuduException While Pair usage in C++ is fairly common, it's a little less common in Java. To improve readability, I'd define this as a nested static class. Also, prefer interface types over implementation types (i.e. List instead of ArrayList). http://gerrit.cloudera.org:8080/#/c/12279/2/java/kudu-client/src/main/java/org/apache/kudu/client/AuthzTokenCache.java@60 PS2, Line 60: retriesForTable = new HashMap<>(); Should probably add a comment explaining why this can't be a concurrent map (sans lock) like authzTokens. AFAICT, it's because it actually would need to be a concurrent multimap that supported a "put and return the number of inserted key-value pairs matching the key I just inserted" method (in order to handle the "send an RPC or just wait with everyone else" stuff in NewAuthzTokenErrB). http://gerrit.cloudera.org:8080/#/c/12279/2/java/kudu-client/src/main/java/org/apache/kudu/client/AuthzTokenCache.java@110 PS2, Line 110: */ Doc the @return. Elsewhere too. http://gerrit.cloudera.org:8080/#/c/12279/2/java/kudu-client/src/main/java/org/apache/kudu/client/AuthzTokenCache.java@137 PS2, Line 137: client.getMasterTable(), tableId, null, true); Nit: annotate what 'true' means. http://gerrit.cloudera.org:8080/#/c/12279/2/java/kudu-client/src/main/java/org/apache/kudu/client/AuthzTokenCache.java@168 PS2, Line 168: if (resp.getAuthzToken() != null) { : LOG.debug("retrieved authz token for " + tableId); : put(tableId, resp.getAuthzToken()); : } else { : // If we were talking to an old master, we would hit an exception. : throw new NonRecoverableException( : Status.InvalidArgument("no authz token retrieved for " + tableId)); : } Nit: invert to simplify control flow: if (resp.getAuthzToken() == null) { throw exception } LOG.debug(...) put(...) http://gerrit.cloudera.org:8080/#/c/12279/1/java/kudu-client/src/test/java/org/apache/kudu/client/TestAuthTokenReacquire.java File java/kudu-client/src/test/java/org/apache/kudu/client/TestAuthTokenReacquire.java: http://gerrit.cloudera.org:8080/#/c/12279/1/java/kudu-client/src/test/java/org/apache/kudu/client/TestAuthTokenReacquire.java@99 PS1, Line 99: + 1 > Was originally because I was worried this wouldn't always expire the token, Looks like the +1 remained, presumably because you found out it is actually necessary? http://gerrit.cloudera.org:8080/#/c/12279/2/java/kudu-client/src/test/java/org/apache/kudu/client/TestAuthzTokenCache.java File java/kudu-client/src/test/java/org/apache/kudu/client/TestAuthzTokenCache.java: http://gerrit.cloudera.org:8080/#/c/12279/2/java/kudu-client/src/test/java/org/apache/kudu/client/TestAuthzTokenCache.java@52 PS2, Line 52: .numMasterServers(3); This is the default; you can drop it. http://gerrit.cloudera.org:8080/#/c/12279/2/java/kudu-client/src/test/java/org/apache/kudu/client/TestMultiMasterAuthzTokens.java File java/kudu-client/src/test/java/org/apache/kudu/client/TestMultiMasterAuthzTokens.java: http://gerrit.cloudera.org:8080/#/c/12279/2/java/kudu-client/src/test/java/org/apache/kudu/client/TestMultiMasterAuthzTokens.java@29 PS2, Line 29: import java.util.concurrent.*; Could you expand this, and the wildcard below too? http://gerrit.cloudera.org:8080/#/c/12279/2/java/kudu-client/src/test/java/org/apache/kudu/client/TestMultiMasterAuthzTokens.java@121 PS2, Line 121: final int TEST_RUNTIME_MS = 60000; That's a really long time for one test. http://gerrit.cloudera.org:8080/#/c/12279/2/java/kudu-client/src/test/java/org/apache/kudu/client/TestMultiMasterAuthzTokens.java@126 PS2, Line 126: ArrayList Just List http://gerrit.cloudera.org:8080/#/c/12279/2/java/kudu-client/src/test/java/org/apache/kudu/client/TestMultiMasterAuthzTokens.java@163 PS2, Line 163: assertTrue(countRowsInTable(table) >= 0); If this fails, does it propagate properly to the rest of the test? http://gerrit.cloudera.org:8080/#/c/12279/2/java/kudu-client/src/test/java/org/apache/kudu/client/TestMultiMasterAuthzTokens.java@175 PS2, Line 175: LOG.debug("Unwrapping exception"); Probably unnecessary. -- To view, visit http://gerrit.cloudera.org:8080/12279 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Iadd5f7709b45628d7ddd9e2b100d0dfaabbf15af Gerrit-Change-Number: 12279 Gerrit-PatchSet: 2 Gerrit-Owner: Andrew Wong <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Andrew Wong <[email protected]> Gerrit-Reviewer: Hao Hao <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Fri, 22 Feb 2019 20:39:29 +0000 Gerrit-HasComments: Yes
