You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Andrew Wong (Code Review)" <ge...@cloudera.org> on 2019/01/26 10:34:12 UTC
[kudu-CR] KUDU-2543 pt 3 java: pass around authz tokens
Andrew Wong has uploaded this change for review. ( http://gerrit.cloudera.org:8080/12279
Change subject: KUDU-2543 pt 3 java: pass around authz tokens
......................................................................
KUDU-2543 pt 3 java: pass around authz tokens
Adds handling of authz tokens to the Java client. The Java client will
now cache tokens upon opening a table, and use them for RPCs that need
them (e.g. Writes and Scans), reacquiring them when receiving word that
they are expired.
This is tested as follows:
- TestAuthnTokenReacquire's test for scans and writes is repurposed to
also test for reacquisition of authz tokens when they expire
- basic tests are added to test the token cache against a single master
- a test is added to test authz reacquisition in the case that a
multi-master deployment undergoes a leadership change
Change-Id: Iadd5f7709b45628d7ddd9e2b100d0dfaabbf15af
---
M java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java
M java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduScanner.java
A java/kudu-client/src/main/java/org/apache/kudu/client/AuthzTokenCache.java
M java/kudu-client/src/main/java/org/apache/kudu/client/Batch.java
M java/kudu-client/src/main/java/org/apache/kudu/client/Connection.java
M java/kudu-client/src/main/java/org/apache/kudu/client/GetTableSchemaRequest.java
M java/kudu-client/src/main/java/org/apache/kudu/client/GetTableSchemaResponse.java
A java/kudu-client/src/main/java/org/apache/kudu/client/InvalidAuthzTokenException.java
M java/kudu-client/src/main/java/org/apache/kudu/client/KuduRpc.java
M java/kudu-client/src/main/java/org/apache/kudu/client/Operation.java
M java/kudu-client/src/main/java/org/apache/kudu/client/RpcProxy.java
R java/kudu-client/src/test/java/org/apache/kudu/client/TestAuthTokenReacquire.java
A java/kudu-client/src/test/java/org/apache/kudu/client/TestAuthzTokenCache.java
A java/kudu-client/src/test/java/org/apache/kudu/client/TestMultiMasterAuthzTokens.java
M java/kudu-test-utils/src/main/java/org/apache/kudu/test/ClientTestUtil.java
15 files changed, 721 insertions(+), 41 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/79/12279/1
--
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: newchange
Gerrit-Change-Id: Iadd5f7709b45628d7ddd9e2b100d0dfaabbf15af
Gerrit-Change-Number: 12279
Gerrit-PatchSet: 1
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
[kudu-CR] KUDU-2543 pt 3 java: pass around authz tokens
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
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 3:
(2 comments)
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();
> IIUC, the different flush modes exercise different code paths though; one u
My concern was more about flushing the session at each row, (unintentionally?) producing the behavior of AUTO_FLUSH_SYNC mode regardless of the 'mode' parameter. If you want that behavior for some reason, please add a comment why flushing per row is necessary regardless of the flush mode.
http://gerrit.cloudera.org:8080/#/c/12279/3/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/3/java/kudu-client/src/test/java/org/apache/kudu/client/TestMultiMasterAuthzTokens.java@71
PS3, Line 71: session.flush();
Still flushing at each row regardless of the 'mode' parameter. Is this intentional?
--
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 <aw...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Thu, 28 Feb 2019 17:34:26 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2543 pt 3 java: pass around authz tokens
Posted by "Hao Hao (Code Review)" <ge...@cloudera.org>.
Hao Hao 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:
(4 comments)
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)
Why do you think this is needed? The authz token errors are not all handled in the RPC layers?
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: .*
nit: avoid '*' for import.
http://gerrit.cloudera.org:8080/#/c/12279/1/java/kudu-client/src/main/java/org/apache/kudu/client/AuthzTokenCache.java@62
PS1, Line 62: tablet
Table here as well?
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?
--
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 <aw...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Fri, 08 Feb 2019 19:54:59 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2543 pt 3 java: pass around authz tokens
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/12279 )
Change subject: KUDU-2543 pt 3 java: pass around authz tokens
......................................................................
KUDU-2543 pt 3 java: pass around authz tokens
Adds handling of authz tokens to the Java client. The Java client will
now cache tokens upon opening a table, and use them for RPCs that need
them (e.g. Writes and Scans), reacquiring them when receiving word that
they are expired.
This is tested as follows:
- TestAuthnTokenReacquire's test for scans and writes is repurposed to
also test for reacquisition of authz tokens when they expire
- basic tests are added to test the token cache
- a test is added to test authz reacquisition in the case that a
multi-master deployment undergoes a leadership change
- a test is added to test authz reacquisition upon invalid or expired
tokens during prolonged workloads against a multi-master deployment
Change-Id: Iadd5f7709b45628d7ddd9e2b100d0dfaabbf15af
Reviewed-on: http://gerrit.cloudera.org:8080/12279
Tested-by: Kudu Jenkins
Reviewed-by: Adar Dembo <ad...@cloudera.com>
Reviewed-by: Alexey Serbin <as...@cloudera.com>
Reviewed-by: Hao Hao <ha...@cloudera.com>
---
M java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java
M java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduScanner.java
A java/kudu-client/src/main/java/org/apache/kudu/client/AuthzTokenCache.java
M java/kudu-client/src/main/java/org/apache/kudu/client/Batch.java
M java/kudu-client/src/main/java/org/apache/kudu/client/Connection.java
M java/kudu-client/src/main/java/org/apache/kudu/client/GetTableSchemaRequest.java
M java/kudu-client/src/main/java/org/apache/kudu/client/GetTableSchemaResponse.java
A java/kudu-client/src/main/java/org/apache/kudu/client/InvalidAuthzTokenException.java
M java/kudu-client/src/main/java/org/apache/kudu/client/KuduRpc.java
M java/kudu-client/src/main/java/org/apache/kudu/client/Operation.java
M java/kudu-client/src/main/java/org/apache/kudu/client/RpcProxy.java
R java/kudu-client/src/test/java/org/apache/kudu/client/TestAuthTokenReacquire.java
A java/kudu-client/src/test/java/org/apache/kudu/client/TestAuthzTokenCache.java
A java/kudu-client/src/test/java/org/apache/kudu/client/TestMultiMasterAuthzTokens.java
M java/kudu-test-utils/src/main/java/org/apache/kudu/test/ClientTestUtil.java
15 files changed, 873 insertions(+), 44 deletions(-)
Approvals:
Kudu Jenkins: Verified
Adar Dembo: Looks good to me, but someone else must approve
Alexey Serbin: Looks good to me, approved
Hao Hao: Looks good to me, approved
--
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: merged
Gerrit-Change-Id: Iadd5f7709b45628d7ddd9e2b100d0dfaabbf15af
Gerrit-Change-Number: 12279
Gerrit-PatchSet: 6
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] KUDU-2543 pt 3 java: pass around authz tokens
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
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 <aw...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Tue, 29 Jan 2019 20:25:05 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2543 pt 3 java: pass around authz tokens
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
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 <aw...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Fri, 22 Feb 2019 20:39:29 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2543 pt 3 java: pass around authz tokens
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Hello Alexey Serbin, Kudu Jenkins, Adar Dembo, Hao Hao,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/12279
to look at the new patch set (#4).
Change subject: KUDU-2543 pt 3 java: pass around authz tokens
......................................................................
KUDU-2543 pt 3 java: pass around authz tokens
Adds handling of authz tokens to the Java client. The Java client will
now cache tokens upon opening a table, and use them for RPCs that need
them (e.g. Writes and Scans), reacquiring them when receiving word that
they are expired.
This is tested as follows:
- TestAuthnTokenReacquire's test for scans and writes is repurposed to
also test for reacquisition of authz tokens when they expire
- basic tests are added to test the token cache
- a test is added to test authz reacquisition in the case that a
multi-master deployment undergoes a leadership change
- a test is added to test authz reacquisition upon invalid or expired
tokens during prolonged workloads against a multi-master deployment
Change-Id: Iadd5f7709b45628d7ddd9e2b100d0dfaabbf15af
---
M java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java
M java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduScanner.java
A java/kudu-client/src/main/java/org/apache/kudu/client/AuthzTokenCache.java
M java/kudu-client/src/main/java/org/apache/kudu/client/Batch.java
M java/kudu-client/src/main/java/org/apache/kudu/client/Connection.java
M java/kudu-client/src/main/java/org/apache/kudu/client/GetTableSchemaRequest.java
M java/kudu-client/src/main/java/org/apache/kudu/client/GetTableSchemaResponse.java
A java/kudu-client/src/main/java/org/apache/kudu/client/InvalidAuthzTokenException.java
M java/kudu-client/src/main/java/org/apache/kudu/client/KuduRpc.java
M java/kudu-client/src/main/java/org/apache/kudu/client/Operation.java
M java/kudu-client/src/main/java/org/apache/kudu/client/RpcProxy.java
A java/kudu-client/src/test/java/org/apache/kudu/client/TestAuthTokenReacquire.java
D java/kudu-client/src/test/java/org/apache/kudu/client/TestAuthnTokenReacquire.java
A java/kudu-client/src/test/java/org/apache/kudu/client/TestAuthzTokenCache.java
A java/kudu-client/src/test/java/org/apache/kudu/client/TestMultiMasterAuthzTokens.java
M java/kudu-test-utils/src/main/java/org/apache/kudu/test/ClientTestUtil.java
16 files changed, 1,011 insertions(+), 181 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/79/12279/4
--
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: newpatchset
Gerrit-Change-Id: Iadd5f7709b45628d7ddd9e2b100d0dfaabbf15af
Gerrit-Change-Number: 12279
Gerrit-PatchSet: 4
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] KUDU-2543 pt 3 java: pass around authz tokens
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Hello Alexey Serbin, Kudu Jenkins, Adar Dembo, Hao Hao,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/12279
to look at the new patch set (#5).
Change subject: KUDU-2543 pt 3 java: pass around authz tokens
......................................................................
KUDU-2543 pt 3 java: pass around authz tokens
Adds handling of authz tokens to the Java client. The Java client will
now cache tokens upon opening a table, and use them for RPCs that need
them (e.g. Writes and Scans), reacquiring them when receiving word that
they are expired.
This is tested as follows:
- TestAuthnTokenReacquire's test for scans and writes is repurposed to
also test for reacquisition of authz tokens when they expire
- basic tests are added to test the token cache
- a test is added to test authz reacquisition in the case that a
multi-master deployment undergoes a leadership change
- a test is added to test authz reacquisition upon invalid or expired
tokens during prolonged workloads against a multi-master deployment
Change-Id: Iadd5f7709b45628d7ddd9e2b100d0dfaabbf15af
---
M java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java
M java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduScanner.java
A java/kudu-client/src/main/java/org/apache/kudu/client/AuthzTokenCache.java
M java/kudu-client/src/main/java/org/apache/kudu/client/Batch.java
M java/kudu-client/src/main/java/org/apache/kudu/client/Connection.java
M java/kudu-client/src/main/java/org/apache/kudu/client/GetTableSchemaRequest.java
M java/kudu-client/src/main/java/org/apache/kudu/client/GetTableSchemaResponse.java
A java/kudu-client/src/main/java/org/apache/kudu/client/InvalidAuthzTokenException.java
M java/kudu-client/src/main/java/org/apache/kudu/client/KuduRpc.java
M java/kudu-client/src/main/java/org/apache/kudu/client/Operation.java
M java/kudu-client/src/main/java/org/apache/kudu/client/RpcProxy.java
R java/kudu-client/src/test/java/org/apache/kudu/client/TestAuthTokenReacquire.java
A java/kudu-client/src/test/java/org/apache/kudu/client/TestAuthzTokenCache.java
A java/kudu-client/src/test/java/org/apache/kudu/client/TestMultiMasterAuthzTokens.java
M java/kudu-test-utils/src/main/java/org/apache/kudu/test/ClientTestUtil.java
15 files changed, 873 insertions(+), 44 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/79/12279/5
--
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: newpatchset
Gerrit-Change-Id: Iadd5f7709b45628d7ddd9e2b100d0dfaabbf15af
Gerrit-Change-Number: 12279
Gerrit-PatchSet: 5
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] KUDU-2543 pt 3 java: pass around authz tokens
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
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 <aw...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Fri, 22 Feb 2019 08:28:35 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2543 pt 3 java: pass around authz tokens
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Hello Alexey Serbin, Kudu Jenkins, Adar Dembo, Hao Hao,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/12279
to look at the new patch set (#3).
Change subject: KUDU-2543 pt 3 java: pass around authz tokens
......................................................................
KUDU-2543 pt 3 java: pass around authz tokens
Adds handling of authz tokens to the Java client. The Java client will
now cache tokens upon opening a table, and use them for RPCs that need
them (e.g. Writes and Scans), reacquiring them when receiving word that
they are expired.
This is tested as follows:
- TestAuthnTokenReacquire's test for scans and writes is repurposed to
also test for reacquisition of authz tokens when they expire
- basic tests are added to test the token cache
- a test is added to test authz reacquisition in the case that a
multi-master deployment undergoes a leadership change
- a test is added to test authz reacquisition upon invalid or expired
tokens during prolonged workloads against a multi-master deployment
Change-Id: Iadd5f7709b45628d7ddd9e2b100d0dfaabbf15af
---
M java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java
M java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduScanner.java
A java/kudu-client/src/main/java/org/apache/kudu/client/AuthzTokenCache.java
M java/kudu-client/src/main/java/org/apache/kudu/client/Batch.java
M java/kudu-client/src/main/java/org/apache/kudu/client/Connection.java
M java/kudu-client/src/main/java/org/apache/kudu/client/GetTableSchemaRequest.java
M java/kudu-client/src/main/java/org/apache/kudu/client/GetTableSchemaResponse.java
A java/kudu-client/src/main/java/org/apache/kudu/client/InvalidAuthzTokenException.java
M java/kudu-client/src/main/java/org/apache/kudu/client/KuduRpc.java
M java/kudu-client/src/main/java/org/apache/kudu/client/Operation.java
M java/kudu-client/src/main/java/org/apache/kudu/client/RpcProxy.java
A java/kudu-client/src/test/java/org/apache/kudu/client/TestAuthTokenReacquire.java
D java/kudu-client/src/test/java/org/apache/kudu/client/TestAuthnTokenReacquire.java
A java/kudu-client/src/test/java/org/apache/kudu/client/TestAuthzTokenCache.java
A java/kudu-client/src/test/java/org/apache/kudu/client/TestMultiMasterAuthzTokens.java
M java/kudu-test-utils/src/main/java/org/apache/kudu/test/ClientTestUtil.java
16 files changed, 1,011 insertions(+), 181 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/79/12279/3
--
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: newpatchset
Gerrit-Change-Id: Iadd5f7709b45628d7ddd9e2b100d0dfaabbf15af
Gerrit-Change-Number: 12279
Gerrit-PatchSet: 3
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] KUDU-2543 pt 3 java: pass around authz tokens
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
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 4:
(2 comments)
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();
> My concern was more about flushing the session at each row, (unintentionall
Ah sure, I'll fix that. The flush mode would still dictate Batches vs Operations though, right? That's all I really care about for this test.
http://gerrit.cloudera.org:8080/#/c/12279/3/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/3/java/kudu-client/src/test/java/org/apache/kudu/client/TestMultiMasterAuthzTokens.java@71
PS3, Line 71:
> Still flushing at each row regardless of the 'mode' parameter. Is this int
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: 4
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Thu, 28 Feb 2019 18:18:56 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2543 pt 3 java: pass around authz tokens
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
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 <aw...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Fri, 22 Feb 2019 18:57:19 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2543 pt 3 java: pass around authz tokens
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
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 <aw...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Thu, 28 Feb 2019 07:19:18 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2543 pt 3 java: pass around authz tokens
Posted by "Hao Hao (Code Review)" <ge...@cloudera.org>.
Hao Hao 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 5: Code-Review+2
--
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: 5
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Sat, 09 Mar 2019 00:06:15 +0000
Gerrit-HasComments: No
[kudu-CR] KUDU-2543 pt 3 java: pass around authz tokens
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
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 5: Code-Review+1
--
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: 5
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Tue, 05 Mar 2019 00:21:44 +0000
Gerrit-HasComments: No
[kudu-CR] KUDU-2543 pt 3 java: pass around authz tokens
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Hello Alexey Serbin, Kudu Jenkins, Adar Dembo, Hao Hao,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/12279
to look at the new patch set (#2).
Change subject: KUDU-2543 pt 3 java: pass around authz tokens
......................................................................
KUDU-2543 pt 3 java: pass around authz tokens
Adds handling of authz tokens to the Java client. The Java client will
now cache tokens upon opening a table, and use them for RPCs that need
them (e.g. Writes and Scans), reacquiring them when receiving word that
they are expired.
This is tested as follows:
- TestAuthnTokenReacquire's test for scans and writes is repurposed to
also test for reacquisition of authz tokens when they expire
- basic tests are added to test the token cache
- a test is added to test authz reacquisition in the case that a
multi-master deployment undergoes a leadership change
- a test is added to test authz reacquisition upon invalid or expired
tokens during prolonged workloads against a multi-master deployment
Change-Id: Iadd5f7709b45628d7ddd9e2b100d0dfaabbf15af
---
M java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java
M java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduScanner.java
A java/kudu-client/src/main/java/org/apache/kudu/client/AuthzTokenCache.java
M java/kudu-client/src/main/java/org/apache/kudu/client/Batch.java
M java/kudu-client/src/main/java/org/apache/kudu/client/Connection.java
M java/kudu-client/src/main/java/org/apache/kudu/client/GetTableSchemaRequest.java
M java/kudu-client/src/main/java/org/apache/kudu/client/GetTableSchemaResponse.java
A java/kudu-client/src/main/java/org/apache/kudu/client/InvalidAuthzTokenException.java
M java/kudu-client/src/main/java/org/apache/kudu/client/KuduRpc.java
M java/kudu-client/src/main/java/org/apache/kudu/client/Operation.java
M java/kudu-client/src/main/java/org/apache/kudu/client/RpcProxy.java
R java/kudu-client/src/test/java/org/apache/kudu/client/TestAuthTokenReacquire.java
A java/kudu-client/src/test/java/org/apache/kudu/client/TestAuthzTokenCache.java
A java/kudu-client/src/test/java/org/apache/kudu/client/TestMultiMasterAuthzTokens.java
M java/kudu-test-utils/src/main/java/org/apache/kudu/test/ClientTestUtil.java
15 files changed, 850 insertions(+), 44 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/79/12279/2
--
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: newpatchset
Gerrit-Change-Id: Iadd5f7709b45628d7ddd9e2b100d0dfaabbf15af
Gerrit-Change-Number: 12279
Gerrit-PatchSet: 2
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] KUDU-2543 pt 3 java: pass around authz tokens
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
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 5:
(1 comment)
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: au
> Yeah, because the token TTL is 1s, it was possible for roughly 1s to pass,
Nope, that actually made it flaky. To guarantee expiration of tokens, I think we need to wait just past the token TTL.
--
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: 5
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Thu, 28 Feb 2019 18:50:55 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2543 pt 3 java: pass around authz tokens
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
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 5: Code-Review+2
(1 comment)
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();
> Ah sure, I'll fix that. The flush mode would still dictate Batches vs Opera
Thanks. Yep, as I understand the flush mode dictates the way how row operations are batched when sending those to tablet server.
--
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: 5
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Fri, 08 Mar 2019 19:43:52 +0000
Gerrit-HasComments: Yes