You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Alexey Serbin (Code Review)" <ge...@cloudera.org> on 2017/05/06 03:03:28 UTC
[kudu-CR] WIP [java] re-acquire authn token if expired
Alexey Serbin has uploaded a new change for review.
http://gerrit.cloudera.org:8080/6816
Change subject: WIP [java] re-acquire authn token if expired
......................................................................
WIP [java] re-acquire authn token if expired
This patch introduces automatic authn token re-acquisition when current
authn token expires. The client automatically retries the RPC that
hits the token expiration error.
Added a test to exercise the new retry logic for token expiration in
case of a basic workload scenario.
In this commit I also added a few lines of code to retry RPC for the
case if server sends ERROR_UNAVAILABLE error code (which is a broader
version of its ERROR_SERVER_TOO_BUSY counterpart).
WIP: there are a couple of TODOs to address
* synchronization of setting/resetting authn token in the client
context
* a test scenario with more exhaustive workload
Change-Id: Iad5bef1d06708215839037741cd3bc213e130af2
---
M java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java
M java/kudu-client/src/main/java/org/apache/kudu/client/Negotiator.java
M java/kudu-client/src/main/java/org/apache/kudu/client/TabletClient.java
A java/kudu-client/src/test/java/org/apache/kudu/client/TestAuthnTokenReacquire.java
4 files changed, 231 insertions(+), 18 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/16/6816/1
--
To view, visit http://gerrit.cloudera.org:8080/6816
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: Iad5bef1d06708215839037741cd3bc213e130af2
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
[kudu-CR] WIP [java] re-acquire authn token if expired
Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
Mike Percy has posted comments on this change.
Change subject: WIP [java] re-acquire authn token if expired
......................................................................
Patch Set 2:
I didn't review this but I filed a JIRA for this feature: KUDU-2013
--
To view, visit http://gerrit.cloudera.org:8080/6816
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Iad5bef1d06708215839037741cd3bc213e130af2
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No
[kudu-CR] WIP [java] re-acquire authn token if expired
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change.
Change subject: WIP [java] re-acquire authn token if expired
......................................................................
Patch Set 2: Verified+1
Unrelated MultiThreadedTabletTest/4.DeleteAndReinsert flake
--
To view, visit http://gerrit.cloudera.org:8080/6816
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Iad5bef1d06708215839037741cd3bc213e130af2
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No
[kudu-CR] [java] re-acquire authn token if expired
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has abandoned this change.
Change subject: [java] re-acquire authn token if expired
......................................................................
Abandoned
This is superseded by https://gerrit.cloudera.org/#/c/7250/
--
To view, visit http://gerrit.cloudera.org:8080/6816
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: abandon
Gerrit-Change-Id: Iad5bef1d06708215839037741cd3bc213e130af2
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] WIP [java] re-acquire authn token if expired
Posted by "Jean-Daniel Cryans (Code Review)" <ge...@cloudera.org>.
Jean-Daniel Cryans has posted comments on this change.
Change subject: WIP [java] re-acquire authn token if expired
......................................................................
Patch Set 2:
(4 comments)
http://gerrit.cloudera.org:8080/#/c/6816/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:
Line 1019:
In general I'm not a fan of cleanups like that that have nothing to do with the patch as it makes it more likely to conflict with other patches that actually have something to do with the part of the code that's being cleaned up.
Line 1327: // TODO(aserbin): synchronize setting the token between different actors.
I think what we should do for ConnectToCluster is to gate all the different threads on the taking of some atomic object using CAS. Look at how we use the field `flushNotification` in AsyncKuduSession.
http://gerrit.cloudera.org:8080/#/c/6816/2/java/kudu-client/src/main/java/org/apache/kudu/client/TabletClient.java
File java/kudu-client/src/main/java/org/apache/kudu/client/TabletClient.java:
Line 383: lock.lock();
Minor but that should be outside of try in case any unchecked exception gets thrown and then you might not have locked.
PS2, Line 407: state = State.POSTPONED_DISCONNECT;
: lock.unlock();
Wrap in try/catch.
--
To view, visit http://gerrit.cloudera.org:8080/6816
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Iad5bef1d06708215839037741cd3bc213e130af2
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] [java] re-acquire authn token if expired
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has uploaded a new patch set (#3).
Change subject: [java] re-acquire authn token if expired
......................................................................
[java] re-acquire authn token if expired
This patch introduces automatic authn token re-acquisition when current
authn token expires. The client automatically retries the RPC that
hits the token expiration error.
Added a couple of tests to exercise the new retry logic for token
expiration in case of a basic workload scenario.
In this commit I also added a few lines of code to retry RPC for the
case if server sends ERROR_UNAVAILABLE error code (which is a broader
version of its ERROR_SERVER_TOO_BUSY counterpart).
Change-Id: Iad5bef1d06708215839037741cd3bc213e130af2
---
M java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java
M java/kudu-client/src/main/java/org/apache/kudu/client/ConnectToCluster.java
M java/kudu-client/src/main/java/org/apache/kudu/client/Negotiator.java
M java/kudu-client/src/main/java/org/apache/kudu/client/TabletClient.java
A java/kudu-client/src/test/java/org/apache/kudu/client/TestAuthnTokenExpiration.java
A java/kudu-client/src/test/java/org/apache/kudu/client/TestAuthnTokenReacquire.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestNegotiator.java
7 files changed, 398 insertions(+), 24 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/16/6816/3
--
To view, visit http://gerrit.cloudera.org:8080/6816
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Iad5bef1d06708215839037741cd3bc213e130af2
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] WIP [java] re-acquire authn token if expired
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change.
Change subject: WIP [java] re-acquire authn token if expired
......................................................................
Patch Set 2:
(3 comments)
> I didn't review this but I filed a JIRA for this feature: KUDU-2013
Thanks. Having a JIRA item makes it easier to track.
http://gerrit.cloudera.org:8080/#/c/6816/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:
Line 1019:
> In general I'm not a fan of cleanups like that that have nothing to do with
Yep, that makes sense -- let me publish it in a separate changelist. I was not sure it was worth having it as a separate one, but now it's clear that's the way.
http://gerrit.cloudera.org:8080/#/c/6816/2/java/kudu-client/src/main/java/org/apache/kudu/client/TabletClient.java
File java/kudu-client/src/main/java/org/apache/kudu/client/TabletClient.java:
Line 383: lock.lock();
> Minor but that should be outside of try in case any unchecked exception get
Done
PS2, Line 407: state = State.POSTPONED_DISCONNECT;
: lock.unlock();
> Wrap in try/catch.
Done
--
To view, visit http://gerrit.cloudera.org:8080/6816
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Iad5bef1d06708215839037741cd3bc213e130af2
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes