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 1: (37 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: SignedTokenPB getAuthzToken(String tableId) { Javadoc. 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: // XXX(awong): should probably handle authz token errors? Seems like a test waiting to be written. Also, does this apply to the 'else'? If so, may want to move it; it's not clear. http://gerrit.cloudera.org:8080/#/c/12279/1/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduScanner.java@892 PS1, Line 892: private Token.SignedTokenPB authzToken; Likewise re: synchronization. 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@42 PS1, Line 42: public class AuthzTokenCache { Can the various public methods here be reduced to package private visibility? http://gerrit.cloudera.org:8080/#/c/12279/1/java/kudu-client/src/main/java/org/apache/kudu/client/AuthzTokenCache.java@46 PS1, Line 46: /** : * Create a new AuthzTokenCache object. : * : * @param client the Kudu client object with which to send requests. : */ : AuthzTokenCache(AsyncKuduClient client) { : this.client = client; : numRetrievalsSent = new AtomicInteger(0); : } This should be moved below the local state. http://gerrit.cloudera.org:8080/#/c/12279/1/java/kudu-client/src/main/java/org/apache/kudu/client/AuthzTokenCache.java@56 PS1, Line 56: tablet You mean "table ID" here and below, right? http://gerrit.cloudera.org:8080/#/c/12279/1/java/kudu-client/src/main/java/org/apache/kudu/client/AuthzTokenCache.java@60 PS1, Line 60: Nit: one empty line too many? http://gerrit.cloudera.org:8080/#/c/12279/1/java/kudu-client/src/main/java/org/apache/kudu/client/AuthzTokenCache.java@78 PS1, Line 78: such is the burden of the tablet server. Nit: sort of a weird philosophical aside to add here. http://gerrit.cloudera.org:8080/#/c/12279/1/java/kudu-client/src/main/java/org/apache/kudu/client/AuthzTokenCache.java@80 PS1, Line 80: public void put(String tableId, Token.SignedTokenPB token) { Could you do full Javadoc for these methods? i.e. with @param, @return, etc. http://gerrit.cloudera.org:8080/#/c/12279/1/java/kudu-client/src/main/java/org/apache/kudu/client/AuthzTokenCache.java@81 PS1, Line 81: assert token != null; How about Preconditions.checkNotNull instead? Or better yet, @NotNull parameter annotations? http://gerrit.cloudera.org:8080/#/c/12279/1/java/kudu-client/src/main/java/org/apache/kudu/client/AuthzTokenCache.java@111 PS1, Line 111: assert !pendingRetries.isEmpty(); Preconditions instead? Below too. http://gerrit.cloudera.org:8080/#/c/12279/1/java/kudu-client/src/main/java/org/apache/kudu/client/AuthzTokenCache.java@121 PS1, Line 121: private void sendRpcForTable(String tableId, Can we reuse AsyncKuduClient.getTableSchema, with some mild refactoring perhaps? Having two code paths that do the same thing is going to make it hard to keep them in sync. And I think we do want to keep them in sync: for example, the existing version in getTableSchema() clears entries from the table locations cache. http://gerrit.cloudera.org:8080/#/c/12279/1/java/kudu-client/src/main/java/org/apache/kudu/client/AuthzTokenCache.java@161 PS1, Line 161: // Regardless of whether we got a token or not, retry pending RPCs. If we didn't get a token, why bother retrying? Do you expect the master to eventually provide a token? Seems like we're bound to retry forever in that case, eventually timing out the RPC. Also, what if we're talking to an old master that lacks authz support? The token will always be null then, right? http://gerrit.cloudera.org:8080/#/c/12279/1/java/kudu-client/src/main/java/org/apache/kudu/client/AuthzTokenCache.java@164 PS1, Line 164: client.handleRetryableErrorNoDelay(rpc, null); Could we force clients to this function to provide the exception that triggered the retrieval, then pass that here? http://gerrit.cloudera.org:8080/#/c/12279/1/java/kudu-client/src/main/java/org/apache/kudu/client/AuthzTokenCache.java@185 PS1, Line 185: && !parentRpc.deadlineTracker.timedOut() Won't this be checked somewhere along the sendRpcToTablet() code path? http://gerrit.cloudera.org:8080/#/c/12279/1/java/kudu-client/src/main/java/org/apache/kudu/client/AuthzTokenCache.java@186 PS1, Line 186: sendRpcForTable(tableId, parentRpc, cb, this); Will this eventually lead to some backoff too? http://gerrit.cloudera.org:8080/#/c/12279/1/java/kudu-client/src/main/java/org/apache/kudu/client/AuthzTokenCache.java@199 PS1, Line 199: ArrayList<KuduRpc<?>> pendingRetries = retriesForTable.get(tableId); : if (pendingRetries == null) { : // There isn't an in-flight RPC to retrieve a new authz token. : retriesForTable.put(tableId, new ArrayList<KuduRpc<?>>(Arrays.asList(rpc))); Would putIfAbsent simplify this? 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: private Token.SignedTokenPB authzToken; Likewise, why does this need synchronization? 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. 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: authz Nit: use 'authorization' the first time you mention authz, then 'authz' in subsequent mentions. 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() { return false; } Style nit: do we use one-line style for non-noop methods in Java? 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: private Token.SignedTokenPB authzToken; Why does authzToken need protection? Sessions are not reentrant, so why does this aspect of Operation state need to be? 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: LOG.info("binding token for " + rpc.getTable().getTableId()); info() seems too high for this. 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.*; Unroll this into the specific imports you need. 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 static junit.framework.TestCase.assertTrue; I think this is the wrong class; you meant org.junit.Assert.assertTrue, no? http://gerrit.cloudera.org:8080/#/c/12279/1/java/kudu-client/src/test/java/org/apache/kudu/client/TestAuthzTokenCache.java@43 PS1, Line 43: static final Logger LOG = LoggerFactory.getLogger(TestAuthzTokenCache.class); private http://gerrit.cloudera.org:8080/#/c/12279/1/java/kudu-client/src/test/java/org/apache/kudu/client/TestAuthzTokenCache.java@47 PS1, Line 47: .numMasterServers(1); Why? http://gerrit.cloudera.org:8080/#/c/12279/1/java/kudu-client/src/test/java/org/apache/kudu/client/TestAuthzTokenCache.java@85 PS1, Line 85: // Wait a bit so the next token we get will be different. : Thread.sleep(1500); Where is this guaranteed? http://gerrit.cloudera.org:8080/#/c/12279/1/java/kudu-client/src/test/java/org/apache/kudu/client/TestAuthzTokenCache.java@107 PS1, Line 107: List<Thread> threads = new ArrayList<>(); It might be clearer to use an ExecutorService and invokeAll() here; you can call get() on each Future you get back and throw the Exception directly if there is one, and then your Callable need not worry about catching anything. http://gerrit.cloudera.org:8080/#/c/12279/1/java/kudu-client/src/test/java/org/apache/kudu/client/TestAuthzTokenCache.java@110 PS1, Line 110: synchronizedMap Maybe a ConcurrentMap instead? http://gerrit.cloudera.org:8080/#/c/12279/1/java/kudu-client/src/test/java/org/apache/kudu/client/TestAuthzTokenCache.java@138 PS1, Line 138: assertTrue( 0 < numRetrievals); : assertTrue( numRetrievals < NUM_THREADS); Nit: extra spaces here. 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: static final Logger LOG = LoggerFactory.getLogger(TestMultiMasterAuthzTokens.class); private http://gerrit.cloudera.org:8080/#/c/12279/1/java/kudu-client/src/test/java/org/apache/kudu/client/TestMultiMasterAuthzTokens.java@35 PS1, Line 35: .numMasterServers(3) : .numTabletServers(1) Aren't these the defaults? http://gerrit.cloudera.org:8080/#/c/12279/1/java/kudu-client/src/test/java/org/apache/kudu/client/TestMultiMasterAuthzTokens.java@56 PS1, Line 56: private void insertRows(KuduTable table, SessionConfiguration.FlushMode mode, : int startRow, int endRow) throws Exception { Seems like this could be simplified given the two ways in which it is used below. http://gerrit.cloudera.org:8080/#/c/12279/1/java/kudu-client/src/test/java/org/apache/kudu/client/TestMultiMasterAuthzTokens.java@79 PS1, Line 79: // Inject invalid tokens such that operations will be forced to go back to : // the master for an authz token. This comment seems misplaced. http://gerrit.cloudera.org:8080/#/c/12279/1/java/kudu-client/src/test/java/org/apache/kudu/client/TestMultiMasterAuthzTokens.java@83 PS1, Line 83: // Do the same for batches of inserts. Also not understanding this comment. Also, isn't this part of the test just a repeat of the part above it? What's the difference? 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: return lhs.toString().compareTo(rhs.toString()) == 0; Is toString() on a PB message guaranteed to produce a unique string for a given message? PB docs aren't clear on this. -- 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: 1 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: Tue, 29 Jan 2019 20:25:05 +0000 Gerrit-HasComments: Yes
