You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Jean-Daniel Cryans (Code Review)" <ge...@cloudera.org> on 2016/10/27 20:31:22 UTC
[kudu-CR] [java client] Remove timeouts when joining in the sync client
Hello Adar Dembo, Alexey Serbin,
I'd like you to do a code review. Please visit
http://gerrit.cloudera.org:8080/4871
to review the following change.
Change subject: [java client] Remove timeouts when joining in the sync client
......................................................................
[java client] Remove timeouts when joining in the sync client
Folks have often seen the following exception:
CAUSED BY: TimeoutException: Timed out after 1ms when joining
followed by some gibberish about Deferreds. What happens here is that we short-circuit
the operation and we return to the user exactly when the timeout they provided expire.
This is nice because it means we respect their deadline to the millisecond. But it's
not nice because we don't know what's going on with their operation and in many cases
we have nothing to interrogate.
This patch removes the timeout when calling Deferred.join() so that the timeout instead
comes from the client internals with all the context we need. The two downsides are 1)
we will not be able to respect their timeout as closely and 2) if there's a bug in the
client then it might join forever. The latter means we expose bugs, so it's not that bad.
This patch also changes DeferredGroupException handling to make it more useful.
Change-Id: Iff4c0783e3c338d268ee1e37b000d45517b3880b
---
M java/kudu-client/src/main/java/org/apache/kudu/client/KuduClient.java
M java/kudu-client/src/main/java/org/apache/kudu/client/KuduException.java
M java/kudu-client/src/main/java/org/apache/kudu/client/KuduScanner.java
M java/kudu-client/src/main/java/org/apache/kudu/client/KuduSession.java
4 files changed, 15 insertions(+), 31 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/71/4871/1
--
To view, visit http://gerrit.cloudera.org:8080/4871
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: Iff4c0783e3c338d268ee1e37b000d45517b3880b
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
[kudu-CR] [java client] Remove timeouts when joining in the sync client
Posted by "Jean-Daniel Cryans (Code Review)" <ge...@cloudera.org>.
Jean-Daniel Cryans has posted comments on this change.
Change subject: [java client] Remove timeouts when joining in the sync client
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/4871/1//COMMIT_MSG
Commit Message:
Line 22: client then it might join forever. The latter means we expose bugs, so it's not that bad.
> could we do something like add 10 seconds to the timeout and join with that
I was inclined to use an even bigger number TBH, but the problem is that we have to use that number every time we join which I thought was kinda messy.
--
To view, visit http://gerrit.cloudera.org:8080/4871
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Iff4c0783e3c338d268ee1e37b000d45517b3880b
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] [java client] Remove timeouts when joining in the sync client
Posted by "Jean-Daniel Cryans (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/4871
to look at the new patch set (#3).
Change subject: [java client] Remove timeouts when joining in the sync client
......................................................................
[java client] Remove timeouts when joining in the sync client
Folks have often seen the following exception:
CAUSED BY: TimeoutException: Timed out after 1ms when joining
followed by some gibberish about Deferreds. What happens here is that we short-circuit
the operation and we return to the user exactly when the timeout they provided expire.
This is nice because it means we respect their deadline to the millisecond. But it's
not nice because we don't know what's going on with their operation and in many cases
we have nothing to interrogate.
This patch removes the timeout when calling Deferred.join() so that the timeout instead
comes from the client internals with all the context we need. The two downsides are 1)
we will not be able to respect their timeout as closely and 2) if there's a bug in the
client then it might join forever. The latter means we expose bugs, so it's not that bad.
This patch also changes DeferredGroupException handling to make it more useful.
Change-Id: Iff4c0783e3c338d268ee1e37b000d45517b3880b
---
M java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java
M java/kudu-client/src/main/java/org/apache/kudu/client/KuduClient.java
M java/kudu-client/src/main/java/org/apache/kudu/client/KuduException.java
M java/kudu-client/src/main/java/org/apache/kudu/client/KuduScanner.java
M java/kudu-client/src/main/java/org/apache/kudu/client/KuduSession.java
5 files changed, 17 insertions(+), 33 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/71/4871/3
--
To view, visit http://gerrit.cloudera.org:8080/4871
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Iff4c0783e3c338d268ee1e37b000d45517b3880b
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] [java client] Remove timeouts when joining in the sync client
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change.
Change subject: [java client] Remove timeouts when joining in the sync client
......................................................................
Patch Set 2:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/4871/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:
PS2, Line 1427: and waits for the configured admin timeout
The change to this line should be in this patch too, right?
--
To view, visit http://gerrit.cloudera.org:8080/4871
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Iff4c0783e3c338d268ee1e37b000d45517b3880b
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] [java client] Remove timeouts when joining in the sync client
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change.
Change subject: [java client] Remove timeouts when joining in the sync client
......................................................................
Patch Set 3: Code-Review+2
Looks good to me, but maybe Todd has some more comments?
--
To view, visit http://gerrit.cloudera.org:8080/4871
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Iff4c0783e3c338d268ee1e37b000d45517b3880b
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No
[kudu-CR] [java client] Remove timeouts when joining in the sync client
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change.
Change subject: [java client] Remove timeouts when joining in the sync client
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/4871/1//COMMIT_MSG
Commit Message:
Line 22: client then it might join forever. The latter means we expose bugs, so it's not that bad.
could we do something like add 10 seconds to the timeout and join with that? that way, in most cases we'll get the real info, but in the case of a bug we at least dont wait forever?
--
To view, visit http://gerrit.cloudera.org:8080/4871
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Iff4c0783e3c338d268ee1e37b000d45517b3880b
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] [java client] Remove timeouts when joining in the sync client
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change.
Change subject: [java client] Remove timeouts when joining in the sync client
......................................................................
Patch Set 3: Code-Review+1
--
To view, visit http://gerrit.cloudera.org:8080/4871
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Iff4c0783e3c338d268ee1e37b000d45517b3880b
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No
[kudu-CR] [java client] Remove timeouts when joining in the sync client
Posted by "Jean-Daniel Cryans (Code Review)" <ge...@cloudera.org>.
Jean-Daniel Cryans has submitted this change and it was merged.
Change subject: [java client] Remove timeouts when joining in the sync client
......................................................................
[java client] Remove timeouts when joining in the sync client
Folks have often seen the following exception:
CAUSED BY: TimeoutException: Timed out after 1ms when joining
followed by some gibberish about Deferreds. What happens here is that we short-circuit
the operation and we return to the user exactly when the timeout they provided expire.
This is nice because it means we respect their deadline to the millisecond. But it's
not nice because we don't know what's going on with their operation and in many cases
we have nothing to interrogate.
This patch removes the timeout when calling Deferred.join() so that the timeout instead
comes from the client internals with all the context we need. The two downsides are 1)
we will not be able to respect their timeout as closely and 2) if there's a bug in the
client then it might join forever. The latter means we expose bugs, so it's not that bad.
This patch also changes DeferredGroupException handling to make it more useful.
Change-Id: Iff4c0783e3c338d268ee1e37b000d45517b3880b
Reviewed-on: http://gerrit.cloudera.org:8080/4871
Reviewed-by: Adar Dembo <ad...@cloudera.com>
Tested-by: Kudu Jenkins
Reviewed-by: Todd Lipcon <to...@apache.org>
---
M java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java
M java/kudu-client/src/main/java/org/apache/kudu/client/KuduClient.java
M java/kudu-client/src/main/java/org/apache/kudu/client/KuduException.java
M java/kudu-client/src/main/java/org/apache/kudu/client/KuduScanner.java
M java/kudu-client/src/main/java/org/apache/kudu/client/KuduSession.java
5 files changed, 17 insertions(+), 33 deletions(-)
Approvals:
Adar Dembo: Looks good to me, approved
Todd Lipcon: Looks good to me, but someone else must approve
Kudu Jenkins: Verified
--
To view, visit http://gerrit.cloudera.org:8080/4871
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: merged
Gerrit-Change-Id: Iff4c0783e3c338d268ee1e37b000d45517b3880b
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] [java client] Remove timeouts when joining in the sync client
Posted by "Jean-Daniel Cryans (Code Review)" <ge...@cloudera.org>.
Jean-Daniel Cryans has posted comments on this change.
Change subject: [java client] Remove timeouts when joining in the sync client
......................................................................
Patch Set 2:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/4871/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:
PS2, Line 1427: and waits for the configured admin timeout
> The change to this line should be in this patch too, right?
Done
--
To view, visit http://gerrit.cloudera.org:8080/4871
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Iff4c0783e3c338d268ee1e37b000d45517b3880b
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] [java client] Remove timeouts when joining in the sync client
Posted by "Jean-Daniel Cryans (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/4871
to look at the new patch set (#2).
Change subject: [java client] Remove timeouts when joining in the sync client
......................................................................
[java client] Remove timeouts when joining in the sync client
Folks have often seen the following exception:
CAUSED BY: TimeoutException: Timed out after 1ms when joining
followed by some gibberish about Deferreds. What happens here is that we short-circuit
the operation and we return to the user exactly when the timeout they provided expire.
This is nice because it means we respect their deadline to the millisecond. But it's
not nice because we don't know what's going on with their operation and in many cases
we have nothing to interrogate.
This patch removes the timeout when calling Deferred.join() so that the timeout instead
comes from the client internals with all the context we need. The two downsides are 1)
we will not be able to respect their timeout as closely and 2) if there's a bug in the
client then it might join forever. The latter means we expose bugs, so it's not that bad.
This patch also changes DeferredGroupException handling to make it more useful.
Change-Id: Iff4c0783e3c338d268ee1e37b000d45517b3880b
---
M java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java
M java/kudu-client/src/main/java/org/apache/kudu/client/KuduClient.java
M java/kudu-client/src/main/java/org/apache/kudu/client/KuduException.java
M java/kudu-client/src/main/java/org/apache/kudu/client/KuduScanner.java
M java/kudu-client/src/main/java/org/apache/kudu/client/KuduSession.java
5 files changed, 16 insertions(+), 32 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/71/4871/2
--
To view, visit http://gerrit.cloudera.org:8080/4871
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Iff4c0783e3c338d268ee1e37b000d45517b3880b
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] [java client] Remove timeouts when joining in the sync client
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change.
Change subject: [java client] Remove timeouts when joining in the sync client
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/4871/1/java/kudu-client/src/main/java/org/apache/kudu/client/KuduException.java
File java/kudu-client/src/main/java/org/apache/kudu/client/KuduException.java:
PS1, Line 89: others exception
other exceptions
--
To view, visit http://gerrit.cloudera.org:8080/4871
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Iff4c0783e3c338d268ee1e37b000d45517b3880b
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes