Alexey Serbin 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: (11 comments) I did a quick pass. Overall looks great, just some nits for this pass. 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. http://gerrit.cloudera.org:8080/#/c/12279/2/java/kudu-client/src/main/java/org/apache/kudu/client/AuthzTokenCache.java@42 PS2, Line 42: the a ? http://gerrit.cloudera.org:8080/#/c/12279/2/java/kudu-client/src/main/java/org/apache/kudu/client/AuthzTokenCache.java@73 PS2, Line 73: numRetrievalsSent = new AtomicInteger(0); Do you expect the counter to overflow? Maybe, using AtomicLong would be a better choice if you are about to use it elsewhere but tests. 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: if (rpc.needsAuthzToken()) { : rpc.bindAuthzToken(client.getAuthzToken(rpc.getTable().getTableId())); : } I remember there was an ugly hack to use this class as a vehicle to make requests to masters as well (table id would be empty in this case). I'm not sure it's still the case, but if so, are you sure this code will handle those cases appropriately? 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@66 PS2, Line 66: KuduSession session = client.newSession(); Why is it necessary to create a new session per row? In real world it seems wasteful; maybe add a comment to explain using a-session-per-row approach in the test? This looks a bit odd given the flush mode as a parameter: effectively, this way of working with write operations means regardless of flush mode it's a sort of AUTO_FLUSH_SYNC. http://gerrit.cloudera.org:8080/#/c/12279/2/java/kudu-client/src/test/java/org/apache/kudu/client/TestMultiMasterAuthzTokens.java@81 PS2, Line 81: KuduSession session = client.newSession(); ditto: why to create a session per row? Maybe, add a comment with explanation at least? http://gerrit.cloudera.org:8080/#/c/12279/2/java/kudu-client/src/test/java/org/apache/kudu/client/TestMultiMasterAuthzTokens.java@99 PS2, Line 99: LOG.debug("Inserting without batching"); Do these messages add any value? Our tests are already too chatty and just flood the console with useless output. 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. http://gerrit.cloudera.org:8080/#/c/12279/2/java/kudu-client/src/test/java/org/apache/kudu/client/TestMultiMasterAuthzTokens.java@120 PS2, Line 120: without interruption. This sounds a bit vague to me. What does this test tries to verify? Why to run multiple threads? http://gerrit.cloudera.org:8080/#/c/12279/2/java/kudu-client/src/test/java/org/apache/kudu/client/TestMultiMasterAuthzTokens.java@148 PS2, Line 148: // Also send writes with batching. : upsertRows(table, AUTO_FLUSH_BACKGROUND, batch * 10, (++batch) * 10); ditto: effectively there is no batching for upsertRows() http://gerrit.cloudera.org:8080/#/c/12279/2/java/kudu-client/src/test/java/org/apache/kudu/client/TestMultiMasterAuthzTokens.java@162 PS2, Line 162: guarantee a row count, but catch any exceptions. If so, then why to wrap the count function with assertTrue( ... >= 0) at all? -- 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 18:57:19 +0000 Gerrit-HasComments: Yes
