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/06/08 22:35:08 UTC
[kudu-CR] [java client] Fix a race in TabletClient cleanup
Jean-Daniel Cryans has uploaded a new change for review.
http://gerrit.cloudera.org:8080/3340
Change subject: [java client] Fix a race in TabletClient cleanup
......................................................................
[java client] Fix a race in TabletClient cleanup
Dan noticed some weird things going on after fixing TestAsyncKuduClient#testDisconnect,
we're either not handling some RPCs or doing it twice.
The reason is that we add to rpcs_inflight directly without caring for the state of
TabletClient (if it's connected or not). Later, we try to detect if we got disconnected
but there was no easy way to tell if our RPC was already retried for us or not.
This patch tries to make it clear whether the RPC at the end of TabletClient#sendRpc
is already handled or not: if the TabletClient is dead, and the rpcs_inflight list is
empty, then it got taken care of. If it's not empty, then it means the RPC needs to
be sent back to the AsyncKuduClient.
This patch fixes flakiness in the test mentioned above.
Change-Id: Ic87425bd54e01a50c3fc11f2862a3e5f737f2bf7
---
M java/kudu-client/src/main/java/org/kududb/client/TabletClient.java
1 file changed, 32 insertions(+), 18 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/40/3340/1
--
To view, visit http://gerrit.cloudera.org:8080/3340
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: Ic87425bd54e01a50c3fc11f2862a3e5f737f2bf7
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Jean-Daniel Cryans
[kudu-CR] [java client] Fix a race in TabletClient cleanup
Posted by "Jean-Daniel Cryans (Code Review)" <ge...@cloudera.org>.
Jean-Daniel Cryans has posted comments on this change.
Change subject: [java client] Fix a race in TabletClient cleanup
......................................................................
Patch Set 2:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/3340/2/java/kudu-client/src/main/java/org/kududb/client/TabletClient.java
File java/kudu-client/src/main/java/org/kududb/client/TabletClient.java:
Line 203: if (tryAgain) {
> Given the flow of L177-197, there's no way that both failRpc and tryAgain c
Ended up like that after a series of edits, I'll put it back together.
Line 678: rpcs = pending_rpcs == null ? new ArrayList<KuduRpc<?>>(rpcs_inflight.size()) : pending_rpcs;
> Nit: can you use "new ArrayList<>(...)" here?
It confuses my IDE.
Line 685: if (rpcs != null) {
> Doesn't seem like rpcs could ever be non-null. Maybe you mean to check if i
My bad.
--
To view, visit http://gerrit.cloudera.org:8080/3340
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic87425bd54e01a50c3fc11f2862a3e5f737f2bf7
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Jean-Daniel Cryans
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Jean-Daniel Cryans
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: Yes
[kudu-CR] [java client] Fix a race in TabletClient cleanup
Posted by "Kudu Jenkins (Code Review)" <ge...@cloudera.org>.
Kudu Jenkins has posted comments on this change.
Change subject: [java client] Fix a race in TabletClient cleanup
......................................................................
Patch Set 2:
Build Started http://104.196.14.100/job/kudu-gerrit/1785/
--
To view, visit http://gerrit.cloudera.org:8080/3340
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic87425bd54e01a50c3fc11f2862a3e5f737f2bf7
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Jean-Daniel Cryans
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Jean-Daniel Cryans
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: No
[kudu-CR] [java client] Fix a race in TabletClient cleanup
Posted by "Jean-Daniel Cryans (Code Review)" <ge...@cloudera.org>.
Jean-Daniel Cryans has posted comments on this change.
Change subject: [java client] Fix a race in TabletClient cleanup
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/3340/1/java/kudu-client/src/main/java/org/kududb/client/TabletClient.java
File java/kudu-client/src/main/java/org/kududb/client/TabletClient.java:
Line 186: LOG.debug("rpcs_inflight is empty and this TabletClient is dead, will assume that whis " +
> Use {} substitution instead of string concat.
Done
--
To view, visit http://gerrit.cloudera.org:8080/3340
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic87425bd54e01a50c3fc11f2862a3e5f737f2bf7
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Jean-Daniel Cryans
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Jean-Daniel Cryans
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: Yes
[kudu-CR] [java client] Fix a race in TabletClient cleanup
Posted by "Kudu Jenkins (Code Review)" <ge...@cloudera.org>.
Kudu Jenkins has posted comments on this change.
Change subject: [java client] Fix a race in TabletClient cleanup
......................................................................
Patch Set 3:
Build Started http://104.196.14.100/job/kudu-gerrit/1807/
--
To view, visit http://gerrit.cloudera.org:8080/3340
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic87425bd54e01a50c3fc11f2862a3e5f737f2bf7
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Jean-Daniel Cryans
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Jean-Daniel Cryans
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: No
[kudu-CR] [java client] Fix a race in TabletClient cleanup
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert has posted comments on this change.
Change subject: [java client] Fix a race in TabletClient cleanup
......................................................................
Patch Set 1: Code-Review+2
(1 comment)
http://gerrit.cloudera.org:8080/#/c/3340/1/java/kudu-client/src/main/java/org/kududb/client/TabletClient.java
File java/kudu-client/src/main/java/org/kududb/client/TabletClient.java:
Line 186: LOG.debug("rpcs_inflight is empty and this TabletClient is dead, will assume that whis " +
Use {} substitution instead of string concat.
--
To view, visit http://gerrit.cloudera.org:8080/3340
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic87425bd54e01a50c3fc11f2862a3e5f737f2bf7
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Jean-Daniel Cryans
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: Yes
[kudu-CR] [java client] Fix a race in TabletClient cleanup
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change.
Change subject: [java client] Fix a race in TabletClient cleanup
......................................................................
Patch Set 2:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/3340/2/java/kudu-client/src/main/java/org/kududb/client/TabletClient.java
File java/kudu-client/src/main/java/org/kududb/client/TabletClient.java:
Line 203: if (tryAgain) {
Given the flow of L177-197, there's no way that both failRpc and tryAgain can be true, so the old code of "if failRpc else if tryAgain" is fine. Is there another reason for changing this down here that I'm not seeing?
Line 678: rpcs = pending_rpcs == null ? new ArrayList<KuduRpc<?>>(rpcs_inflight.size()) : pending_rpcs;
Nit: can you use "new ArrayList<>(...)" here?
Line 685: if (rpcs != null) {
Doesn't seem like rpcs could ever be non-null. Maybe you mean to check if it's empty here?
--
To view, visit http://gerrit.cloudera.org:8080/3340
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic87425bd54e01a50c3fc11f2862a3e5f737f2bf7
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Jean-Daniel Cryans
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Jean-Daniel Cryans
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: Yes
[kudu-CR] [java client] Fix a race in TabletClient cleanup
Posted by "Jean-Daniel Cryans (Code Review)" <ge...@cloudera.org>.
Hello Dan Burkert, Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/3340
to look at the new patch set (#3).
Change subject: [java client] Fix a race in TabletClient cleanup
......................................................................
[java client] Fix a race in TabletClient cleanup
Dan noticed some weird things going on after fixing TestAsyncKuduClient#testDisconnect,
we're either not handling some RPCs or doing it twice.
The reason is that we add to rpcs_inflight directly without caring for the state of
TabletClient (if it's connected or not). Later, we try to detect if we got disconnected
but there was no easy way to tell if our RPC was already retried for us or not.
This patch tries to make it clear whether the RPC at the end of TabletClient#sendRpc
is already handled or not: if the TabletClient is dead, and the rpcs_inflight list is
empty, then it got taken care of. If it's not empty, then it means the RPC needs to
be sent back to the AsyncKuduClient.
This patch fixes flakiness in the test mentioned above.
Change-Id: Ic87425bd54e01a50c3fc11f2862a3e5f737f2bf7
---
M java/kudu-client/src/main/java/org/kududb/client/TabletClient.java
1 file changed, 32 insertions(+), 21 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/40/3340/3
--
To view, visit http://gerrit.cloudera.org:8080/3340
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ic87425bd54e01a50c3fc11f2862a3e5f737f2bf7
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Jean-Daniel Cryans
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Jean-Daniel Cryans
Gerrit-Reviewer: Kudu Jenkins
[kudu-CR] [java client] Fix a race in TabletClient cleanup
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change.
Change subject: [java client] Fix a race in TabletClient cleanup
......................................................................
Patch Set 3: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/3340
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic87425bd54e01a50c3fc11f2862a3e5f737f2bf7
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Jean-Daniel Cryans
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Jean-Daniel Cryans
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: No
[kudu-CR] [java client] Fix a race in TabletClient cleanup
Posted by "Kudu Jenkins (Code Review)" <ge...@cloudera.org>.
Kudu Jenkins has posted comments on this change.
Change subject: [java client] Fix a race in TabletClient cleanup
......................................................................
Patch Set 1:
Build Started http://104.196.14.100/job/kudu-gerrit/1782/
--
To view, visit http://gerrit.cloudera.org:8080/3340
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic87425bd54e01a50c3fc11f2862a3e5f737f2bf7
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Jean-Daniel Cryans
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: No
[kudu-CR] [java client] Fix a race in TabletClient cleanup
Posted by "Jean-Daniel Cryans (Code Review)" <ge...@cloudera.org>.
Hello Dan Burkert, Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/3340
to look at the new patch set (#2).
Change subject: [java client] Fix a race in TabletClient cleanup
......................................................................
[java client] Fix a race in TabletClient cleanup
Dan noticed some weird things going on after fixing TestAsyncKuduClient#testDisconnect,
we're either not handling some RPCs or doing it twice.
The reason is that we add to rpcs_inflight directly without caring for the state of
TabletClient (if it's connected or not). Later, we try to detect if we got disconnected
but there was no easy way to tell if our RPC was already retried for us or not.
This patch tries to make it clear whether the RPC at the end of TabletClient#sendRpc
is already handled or not: if the TabletClient is dead, and the rpcs_inflight list is
empty, then it got taken care of. If it's not empty, then it means the RPC needs to
be sent back to the AsyncKuduClient.
This patch fixes flakiness in the test mentioned above.
Change-Id: Ic87425bd54e01a50c3fc11f2862a3e5f737f2bf7
---
M java/kudu-client/src/main/java/org/kududb/client/TabletClient.java
1 file changed, 32 insertions(+), 18 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/40/3340/2
--
To view, visit http://gerrit.cloudera.org:8080/3340
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ic87425bd54e01a50c3fc11f2862a3e5f737f2bf7
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Jean-Daniel Cryans
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
[kudu-CR] [java client] Fix a race in TabletClient cleanup
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has submitted this change and it was merged.
Change subject: [java client] Fix a race in TabletClient cleanup
......................................................................
[java client] Fix a race in TabletClient cleanup
Dan noticed some weird things going on after fixing TestAsyncKuduClient#testDisconnect,
we're either not handling some RPCs or doing it twice.
The reason is that we add to rpcs_inflight directly without caring for the state of
TabletClient (if it's connected or not). Later, we try to detect if we got disconnected
but there was no easy way to tell if our RPC was already retried for us or not.
This patch tries to make it clear whether the RPC at the end of TabletClient#sendRpc
is already handled or not: if the TabletClient is dead, and the rpcs_inflight list is
empty, then it got taken care of. If it's not empty, then it means the RPC needs to
be sent back to the AsyncKuduClient.
This patch fixes flakiness in the test mentioned above.
Change-Id: Ic87425bd54e01a50c3fc11f2862a3e5f737f2bf7
Reviewed-on: http://gerrit.cloudera.org:8080/3340
Tested-by: Kudu Jenkins
Reviewed-by: Adar Dembo <ad...@cloudera.com>
---
M java/kudu-client/src/main/java/org/kududb/client/TabletClient.java
1 file changed, 32 insertions(+), 21 deletions(-)
Approvals:
Adar Dembo: Looks good to me, approved
Kudu Jenkins: Verified
--
To view, visit http://gerrit.cloudera.org:8080/3340
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: merged
Gerrit-Change-Id: Ic87425bd54e01a50c3fc11f2862a3e5f737f2bf7
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Jean-Daniel Cryans
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Jean-Daniel Cryans
Gerrit-Reviewer: Kudu Jenkins