Andrew Wong 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 3: (27 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: * provided, even if the RPC should be sent by ID. > Nit: annotate what 'false' means? Done http://gerrit.cloudera.org:8080/#/c/12279/2/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java@1937 PS2, Line 1937: * We're in the context of decode() meaning we need to either callback or retry later. > Looks like some plumbing would net us exceptions here in the same way as ha Looking around a bit, seems like more plumbing than I want to sign up for right now. I'll leave a TODO though. http://gerrit.cloudera.org:8080/#/c/12279/2/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java@1952 PS2, Line 1952: : <R> void handleRetryableError(final KuduRpc<R> rpc, KuduException ex) { > authzTokenCache.get() never sends an RPC though. Done 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: return replicaSelection; > Nit: break out. Elsewhere too. Done 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@41 PS2, Line 41: Cache for authz tokens received from the master > Please add a note that this cache has unlimited capacity. Done http://gerrit.cloudera.org:8080/#/c/12279/2/java/kudu-client/src/main/java/org/apache/kudu/client/AuthzTokenCache.java@42 PS2, Line 42: thz > a ? Done http://gerrit.cloudera.org:8080/#/c/12279/2/java/kudu-client/src/main/java/org/apache/kudu/client/AuthzTokenCache.java@59 PS2, Line 59: ggerFactory.getLogger(AuthzTok > While Pair usage in C++ is fairly common, it's a little less common in Java Done http://gerrit.cloudera.org:8080/#/c/12279/2/java/kudu-client/src/main/java/org/apache/kudu/client/AuthzTokenCache.java@60 PS2, Line 60: private final AsyncKuduClient client; > Should probably add a comment explaining why this can't be a concurrent map Done http://gerrit.cloudera.org:8080/#/c/12279/2/java/kudu-client/src/main/java/org/apache/kudu/client/AuthzTokenCache.java@73 PS2, Line 73: retriesForTable = new HashMap<>(); > Do you expect the counter to overflow? Maybe, using AtomicLong would be a This is just used for tests. 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. Done http://gerrit.cloudera.org:8080/#/c/12279/2/java/kudu-client/src/main/java/org/apache/kudu/client/AuthzTokenCache.java@137 PS2, Line 137: /** > Nit: annotate what 'true' means. Done http://gerrit.cloudera.org:8080/#/c/12279/2/java/kudu-client/src/main/java/org/apache/kudu/client/AuthzTokenCache.java@168 PS2, Line 168: * @param rpc the RPC that needs a new authz token : * @param ex error that caused triggered this retrieval : * @param <R> the RPC type : */ : <R> void retrieveAuthzToken(@Nonnull final KuduRpc<R> rpc, @Nonnull final KuduException ex) { : /** : * Handles a response from getting an authz token. : */ > Nit: invert to simplify control flow: Done http://gerrit.cloudera.org:8080/#/c/12279/2/java/kudu-client/src/main/java/org/apache/kudu/client/RpcProxy.java File java/kudu-client/src/main/java/org/apache/kudu/client/RpcProxy.java: http://gerrit.cloudera.org:8080/#/c/12279/2/java/kudu-client/src/main/java/org/apache/kudu/client/RpcProxy.java@170 PS2, Line 170: * @param <R> type of the RPC : * @param client client object to handle response and sending retries, if needed : * > I remember there was an ugly hack to use this class as a vehicle to make re Yeah, that's still the case, and this will work because only the RPCs that require authz tokens will implement `needsAuthzToken` and `bindAuthzToken`. If they don't, the defaults will ignore this code block 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: > Looks like the +1 remained, presumably because you found out it is actually Yeah, because the token TTL is 1s, it was possible for roughly 1s to pass, to get a new token, and for that token to be the same because the token's expiration time has a granularity of seconds. So sleeping for longer did the trick. To make it clearer, I'll just bump the TTL an extra second and add a comment. 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: > This is the default; you can drop it. Done 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.CountDownLatch; > Could you expand this, and the wildcard below too? *shakes fist at annoying IntelliJ settings* http://gerrit.cloudera.org:8080/#/c/12279/2/java/kudu-client/src/test/java/org/apache/kudu/client/TestMultiMasterAuthzTokens.java@66 PS2, Line 66: KuduSession session = client.newSession(); > Why is it necessary to create a new session per row? In real world it seem IIUC, the different flush modes exercise different code paths though; one uses Batches, the other, Operations, right? But noted, I'll pull this out of the loop. http://gerrit.cloudera.org:8080/#/c/12279/2/java/kudu-client/src/test/java/org/apache/kudu/client/TestMultiMasterAuthzTokens.java@81 PS2, Line 81: duSession session = client.newSession(); > ditto: why to create a session per row? Maybe, add a comment with explanat Ack http://gerrit.cloudera.org:8080/#/c/12279/2/java/kudu-client/src/test/java/org/apache/kudu/client/TestMultiMasterAuthzTokens.java@99 PS2, Line 99: > Do these messages add any value? Our tests are already too chatty and just Generally I like adding logs to debug to make a failed log easier to read. But sure, I'll take them out if you prefer. http://gerrit.cloudera.org:8080/#/c/12279/2/java/kudu-client/src/test/java/org/apache/kudu/client/TestMultiMasterAuthzTokens.java@106 PS2, Line 106: insertRows(table, AUTO_FLUSH_BACKGROUND, NUM_REQS, 2 * NUM_REQS); > Effectively there is no batching given how insertRows() works. IIUC, the different flush modes exercise different code paths though; one uses Batches, the other, Operations, right? http://gerrit.cloudera.org:8080/#/c/12279/2/java/kudu-client/src/test/java/org/apache/kudu/client/TestMultiMasterAuthzTokens.java@120 PS2, Line 120: reacquire tokens as n > This sounds a bit vague to me. What does this test tries to verify? Why t Done http://gerrit.cloudera.org:8080/#/c/12279/2/java/kudu-client/src/test/java/org/apache/kudu/client/TestMultiMasterAuthzTokens.java@121 PS2, Line 121: // without surfacing token errors to the client. > That's a really long time for one test. Done http://gerrit.cloudera.org:8080/#/c/12279/2/java/kudu-client/src/test/java/org/apache/kudu/client/TestMultiMasterAuthzTokens.java@126 PS2, Line 126: final Exe > Just List Done http://gerrit.cloudera.org:8080/#/c/12279/2/java/kudu-client/src/test/java/org/apache/kudu/client/TestMultiMasterAuthzTokens.java@148 PS2, Line 148: while (latch.getCount() > 0) { : // Also send writes with batching. > ditto: effectively there is no batching for upsertRows() IIUC, the different flush modes exercise different code paths though; one uses Batches, the other, Operations, right? http://gerrit.cloudera.org:8080/#/c/12279/2/java/kudu-client/src/test/java/org/apache/kudu/client/TestMultiMasterAuthzTokens.java@162 PS2, Line 162: etCount() > 0) { > If so, then why to wrap the count function with assertTrue( ... >= 0) at al Done http://gerrit.cloudera.org:8080/#/c/12279/2/java/kudu-client/src/test/java/org/apache/kudu/client/TestMultiMasterAuthzTokens.java@163 PS2, Line 163: // We can't guarantee a row count, but catch any exceptions. > If this fails, does it propagate properly to the rest of the test? Not sure, but I removed it anyway, since the assertion isn't particularly useful. http://gerrit.cloudera.org:8080/#/c/12279/2/java/kudu-client/src/test/java/org/apache/kudu/client/TestMultiMasterAuthzTokens.java@175 PS2, Line 175: for (Future<Exception> future : exceptions) { > Probably unnecessary. Done -- 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: 3 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: Thu, 28 Feb 2019 07:19:18 +0000 Gerrit-HasComments: Yes
