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 2: (41 comments) http://gerrit.cloudera.org:8080/#/c/12279/1/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/1/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java@1944 PS1, Line 1944: * @param rpc the RPC that failed with an invalid authz token > Javadoc. Done http://gerrit.cloudera.org:8080/#/c/12279/1/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/1/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduScanner.java@577 PS1, Line 577: return Deferred.fromError(e); // Let the error propagate. > Seems like a test waiting to be written. This is an early comment in implementation; should be handled by the other authz token methods in this file. http://gerrit.cloudera.org:8080/#/c/12279/1/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduScanner.java@577 PS1, Line 577: return Defe > Why do you think this is needed? The authz token errors are not all handled This is an early comment in implementation; should be handled by the other authz token methods in this file. http://gerrit.cloudera.org:8080/#/c/12279/1/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduScanner.java@892 PS1, Line 892: this.setTimeoutMillis(scanRequestTimeout); > Likewise re: synchronization. Done http://gerrit.cloudera.org:8080/#/c/12279/1/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/1/java/kudu-client/src/main/java/org/apache/kudu/client/AuthzTokenCache.java@30 PS1, Line 30: ot > nit: avoid '*' for import. Done http://gerrit.cloudera.org:8080/#/c/12279/1/java/kudu-client/src/main/java/org/apache/kudu/client/AuthzTokenCache.java@42 PS1, Line 42: * authz token upon opening the table and put it into the cache. A subsequent > Can the various public methods here be reduced to package private visibilit TIL. http://gerrit.cloudera.org:8080/#/c/12279/1/java/kudu-client/src/main/java/org/apache/kudu/client/AuthzTokenCache.java@46 PS1, Line 46: @ThreadSafe : @InterfaceAudience.Private : public class AuthzTokenCache { : private static final Logger LOG = LoggerFactory.getLogger(AuthzTokenCache.class); : private final AsyncKuduClient client; : : // Map from a table ID to an authz token for that table. : private final ConcurrentHashMap<String, Token.SignedTokenPB> authzTokens = new ConcurrentHashMap<>(); : > This should be moved below the local state. Done http://gerrit.cloudera.org:8080/#/c/12279/1/java/kudu-client/src/main/java/org/apache/kudu/client/AuthzTokenCache.java@56 PS1, Line 56: RPCs > You mean "table ID" here and below, right? Done http://gerrit.cloudera.org:8080/#/c/12279/1/java/kudu-client/src/main/java/org/apache/kudu/client/AuthzTokenCache.java@60 PS1, Line 60: retriesForTable = new HashMap<>(); > Nit: one empty line too many? Done http://gerrit.cloudera.org:8080/#/c/12279/1/java/kudu-client/src/main/java/org/apache/kudu/client/AuthzTokenCache.java@62 PS1, Line 62: > Table here as well? Done http://gerrit.cloudera.org:8080/#/c/12279/1/java/kudu-client/src/main/java/org/apache/kudu/client/AuthzTokenCache.java@78 PS1, Line 78: > Nit: sort of a weird philosophical aside to add here. Done http://gerrit.cloudera.org:8080/#/c/12279/1/java/kudu-client/src/main/java/org/apache/kudu/client/AuthzTokenCache.java@80 PS1, Line 80: @InterfaceAudience.LimitedPrivate("Test") > Could you do full Javadoc for these methods? i.e. with @param, @return, etc Done http://gerrit.cloudera.org:8080/#/c/12279/1/java/kudu-client/src/main/java/org/apache/kudu/client/AuthzTokenCache.java@81 PS1, Line 81: int numRetrievalsSent() { > How about Preconditions.checkNotNull instead? Or better yet, @NotNull param Done http://gerrit.cloudera.org:8080/#/c/12279/1/java/kudu-client/src/main/java/org/apache/kudu/client/AuthzTokenCache.java@111 PS1, Line 111: private ArrayList<Pair<KuduRpc<?>, KuduException>> clearPendingRetries(@Nonnull String tableId) { > Preconditions instead? Below too. Done http://gerrit.cloudera.org:8080/#/c/12279/1/java/kudu-client/src/main/java/org/apache/kudu/client/AuthzTokenCache.java@121 PS1, Line 121: /** > Can we reuse AsyncKuduClient.getTableSchema, with some mild refactoring per I don't think reusing it makes sense. IIUC (medium-sized "if"), the purpose of clearing the locations cache is to ensure the client will fetch partitions upon opening a table -- seems specific to openTable. Also it's worth pointing out that they don't do the same thing; they just happen to use the same RPC. You could imagine in the future, implementing this with its own dedicated RPC endpoint that doesn't return the schema, just a token. http://gerrit.cloudera.org:8080/#/c/12279/1/java/kudu-client/src/main/java/org/apache/kudu/client/AuthzTokenCache.java@161 PS1, Line 161: > If we didn't get a token, why bother retrying? Do you expect the master to Good point, we shouldn't ever expect a healthy response to _not_ have an authz token. I'll throw an error instead. If talking to an old master, we'd get an error (by virtue of required feature flags) and trigger the Errback. http://gerrit.cloudera.org:8080/#/c/12279/1/java/kudu-client/src/main/java/org/apache/kudu/client/AuthzTokenCache.java@164 PS1, Line 164: } > Could we force clients to this function to provide the exception that trigg Done http://gerrit.cloudera.org:8080/#/c/12279/1/java/kudu-client/src/main/java/org/apache/kudu/client/AuthzTokenCache.java@185 PS1, Line 185: > Won't this be checked somewhere along the sendRpcToTablet() code path? Done http://gerrit.cloudera.org:8080/#/c/12279/1/java/kudu-client/src/main/java/org/apache/kudu/client/AuthzTokenCache.java@186 PS1, Line 186: final class NewAuthzTokenErrB implements Callback<Void, Exception> { > Will this eventually lead to some backoff too? Yeah, `sendRpcForTable` is retrieving an authz token via GetTableSchema, and retrying of the GetTableSchema, like other proxy calls, is handled by AsyncKuduClient, delays and all. That said, there aren't delays between failed RPCs -- not sure there's much value to that, given the per-RPC backoff. http://gerrit.cloudera.org:8080/#/c/12279/1/java/kudu-client/src/main/java/org/apache/kudu/client/AuthzTokenCache.java@199 PS1, Line 199: sendRetrievalForRpc(parentRpc, cb, this); : } else { : for (Pair<KuduRpc<?>, KuduException> rpcAndEx : clearPendingRetries(tableId)) { : rpcAndEx.getFirst().errback(e); > Would putIfAbsent simplify this? Done http://gerrit.cloudera.org:8080/#/c/12279/1/java/kudu-client/src/main/java/org/apache/kudu/client/Batch.java File java/kudu-client/src/main/java/org/apache/kudu/client/Batch.java: http://gerrit.cloudera.org:8080/#/c/12279/1/java/kudu-client/src/main/java/org/apache/kudu/client/Batch.java@58 PS1, Line 58: */ > Likewise, why does this need synchronization? Done http://gerrit.cloudera.org:8080/#/c/12279/1/java/kudu-client/src/main/java/org/apache/kudu/client/GetTableSchemaResponse.java File java/kudu-client/src/main/java/org/apache/kudu/client/GetTableSchemaResponse.java: http://gerrit.cloudera.org:8080/#/c/12279/1/java/kudu-client/src/main/java/org/apache/kudu/client/GetTableSchemaResponse.java@34 PS1, Line 34: /** > Update this list. Done http://gerrit.cloudera.org:8080/#/c/12279/1/java/kudu-client/src/main/java/org/apache/kudu/client/InvalidAuthzTokenException.java File java/kudu-client/src/main/java/org/apache/kudu/client/InvalidAuthzTokenException.java: http://gerrit.cloudera.org:8080/#/c/12279/1/java/kudu-client/src/main/java/org/apache/kudu/client/InvalidAuthzTokenException.java@25 PS1, Line 25: autho > Nit: use 'authorization' the first time you mention authz, then 'authz' in Done http://gerrit.cloudera.org:8080/#/c/12279/1/java/kudu-client/src/main/java/org/apache/kudu/client/KuduRpc.java File java/kudu-client/src/main/java/org/apache/kudu/client/KuduRpc.java: http://gerrit.cloudera.org:8080/#/c/12279/1/java/kudu-client/src/main/java/org/apache/kudu/client/KuduRpc.java@125 PS1, Line 125: boolean needsAuthzToken() { > Style nit: do we use one-line style for non-noop methods in Java? Ah, Intellij was displaying these guys wrapped, so I followed suit. http://gerrit.cloudera.org:8080/#/c/12279/1/java/kudu-client/src/main/java/org/apache/kudu/client/Operation.java File java/kudu-client/src/main/java/org/apache/kudu/client/Operation.java: http://gerrit.cloudera.org:8080/#/c/12279/1/java/kudu-client/src/main/java/org/apache/kudu/client/Operation.java@93 PS1, Line 93: > Why does authzToken need protection? Sessions are not reentrant, so why doe Done http://gerrit.cloudera.org:8080/#/c/12279/1/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/1/java/kudu-client/src/main/java/org/apache/kudu/client/RpcProxy.java@171 PS1, Line 171: rpc.bindAuthzToken(client.getAuthzToken(rpc.getTable().getTableId())); > info() seems too high for this. Done 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 > Why we need to +1 here? Was originally because I was worried this wouldn't always expire the token, so to be safe I waited an extra second. Will remove. http://gerrit.cloudera.org:8080/#/c/12279/1/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/1/java/kudu-client/src/test/java/org/apache/kudu/client/TestAuthzTokenCache.java@32 PS1, Line 32: import java.util.concurrent.Callable; > Unroll this into the specific imports you need. Done http://gerrit.cloudera.org:8080/#/c/12279/1/java/kudu-client/src/test/java/org/apache/kudu/client/TestAuthzTokenCache.java@35 PS1, Line 35: import java.util.concurrent.Future; > I think this is the wrong class; you meant org.junit.Assert.assertTrue, no? Done http://gerrit.cloudera.org:8080/#/c/12279/1/java/kudu-client/src/test/java/org/apache/kudu/client/TestAuthzTokenCache.java@43 PS1, Line 43: > private Done http://gerrit.cloudera.org:8080/#/c/12279/1/java/kudu-client/src/test/java/org/apache/kudu/client/TestAuthzTokenCache.java@47 PS1, Line 47: // This tests basic functionality of the authz token cache (e.g. putting > Why? Was for simplicity, this test only needs on master to do its job of testing the cache. Though I suppose having more masters wouldn't hurt. http://gerrit.cloudera.org:8080/#/c/12279/1/java/kudu-client/src/test/java/org/apache/kudu/client/TestAuthzTokenCache.java@85 PS1, Line 85: // upon accessing a table. : final KuduTable tab > Where is this guaranteed? Done http://gerrit.cloudera.org:8080/#/c/12279/1/java/kudu-client/src/test/java/org/apache/kudu/client/TestAuthzTokenCache.java@107 PS1, Line 107: assertTrue(asyncClient.getAuthzToken(tableId).equals(originalToken)); > It might be clearer to use an ExecutorService and invokeAll() here; you can Done http://gerrit.cloudera.org:8080/#/c/12279/1/java/kudu-client/src/test/java/org/apache/kudu/client/TestAuthzTokenCache.java@110 PS1, Line 110: > Maybe a ConcurrentMap instead? Done http://gerrit.cloudera.org:8080/#/c/12279/1/java/kudu-client/src/test/java/org/apache/kudu/client/TestAuthzTokenCache.java@138 PS1, Line 138: if (e != null) { : fails++; > Nit: extra spaces here. Done http://gerrit.cloudera.org:8080/#/c/12279/1/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/1/java/kudu-client/src/test/java/org/apache/kudu/client/TestMultiMasterAuthzTokens.java@32 PS1, Line 32: import static org.apache.kudu.client.SessionConfiguration.FlushMode.AUTO_FLUSH_SYNC; > private Done http://gerrit.cloudera.org:8080/#/c/12279/1/java/kudu-client/src/test/java/org/apache/kudu/client/TestMultiMasterAuthzTokens.java@35 PS1, Line 35: import static org.junit.Assert.assertTrue; : > Aren't these the defaults? Done http://gerrit.cloudera.org:8080/#/c/12279/1/java/kudu-client/src/test/java/org/apache/kudu/client/TestMultiMasterAuthzTokens.java@56 PS1, Line 56: @Rule : public KuduTestHarness harness = new KuduTestHarness(clusterBuilder) > Seems like this could be simplified given the two ways in which it is used Yeah, meant to use it differently. http://gerrit.cloudera.org:8080/#/c/12279/1/java/kudu-client/src/test/java/org/apache/kudu/client/TestMultiMasterAuthzTokens.java@79 PS1, Line 79: int startRow, int endRow) throws Exception { : for (int i = startRow; i < endRow > This comment seems misplaced. Done http://gerrit.cloudera.org:8080/#/c/12279/1/java/kudu-client/src/test/java/org/apache/kudu/client/TestMultiMasterAuthzTokens.java@83 PS1, Line 83: Upsert upsert = createBasicSchemaUpsert(table, i); > Also not understanding this comment. Yeah, meant to use batches via AUTO_FLUSH_BACKGROUND. http://gerrit.cloudera.org:8080/#/c/12279/1/java/kudu-test-utils/src/main/java/org/apache/kudu/test/ClientTestUtil.java File java/kudu-test-utils/src/main/java/org/apache/kudu/test/ClientTestUtil.java: http://gerrit.cloudera.org:8080/#/c/12279/1/java/kudu-test-utils/src/main/java/org/apache/kudu/test/ClientTestUtil.java@379 PS1, Line 379: .precision(DecimalUtil.MAX_DECIMAL64_PRECISION).build() > Is toString() on a PB message guaranteed to produce a unique string for a g Not sure, but SignedTokenPB.equals() is actually overriden and makes this function not very useful. -- 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 08:28:35 +0000 Gerrit-HasComments: Yes
