[kudu-CR] [java client] Fix a race in TabletClient cleanup
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 --- 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 Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Jean-Daniel Cryans Gerrit-Reviewer: Kudu Jenkins
[kudu-CR] [java client] Fix a race in TabletClient cleanup
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 Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Jean-Daniel Cryans Gerrit-Reviewer: Kudu Jenkins Gerrit-HasComments: No
[kudu-CR] [java client] Fix a race in TabletClient cleanup
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 Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Jean-Daniel Cryans Gerrit-Reviewer: Kudu Jenkins Gerrit-HasComments: No
[kudu-CR] [java client] Fix a race in TabletClient cleanup
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 Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Jean-Daniel Cryans Gerrit-Reviewer: Kudu Jenkins
[kudu-CR] [java client] Fix a race in TabletClient cleanup
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>(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 Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Jean-Daniel Cryans Gerrit-Reviewer: Kudu Jenkins Gerrit-HasComments: Yes
[kudu-CR] [java client] Fix a race in TabletClient cleanup
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>(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 Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Jean-Daniel Cryans Gerrit-Reviewer: Kudu Jenkins Gerrit-HasComments: Yes
[kudu-CR] [java client] Fix a race in TabletClient cleanup
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 Gerrit-Reviewer: Jean-Daniel Cryans Gerrit-Reviewer: Kudu Jenkins Gerrit-HasComments: No
[kudu-CR] [java client] Fix a race in TabletClient cleanup
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 Gerrit-Reviewer: Kudu Jenkins
[kudu-CR] [java client] Fix a race in TabletClient cleanup
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 Gerrit-Reviewer: Jean-Daniel Cryans Gerrit-Reviewer: Kudu Jenkins Gerrit-HasComments: Yes
[kudu-CR] [java client] Fix a race in TabletClient cleanup
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 Gerrit-Reviewer: Kudu Jenkins Gerrit-HasComments: Yes
[kudu-CR] [java client] Fix a race in TabletClient cleanup
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
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