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/07/22 02:11:25 UTC
[kudu-CR] [java client] Fix a race involving the read timeout
Jean-Daniel Cryans has uploaded a new change for review.
http://gerrit.cloudera.org:8080/3724
Change subject: [java client] Fix a race involving the read timeout
......................................................................
[java client] Fix a race involving the read timeout
The ReadTimeoutHanlder isn't something we really want (per-RPC timer
would be better than socket read timeout), but some handling that was
added a year ago (01698c3) made things worse. Being the author of that other
patch, I think my fix was misguided.
The race this patch fixes is that some RPCs were able send their payload
on the wire even after cleanup() had run, but before `chan` was set to
null, when a ReadTimeoutException was fired. The proper way is to close
the channel first, then start cleaning up, so that concurrent threads who
process RPCs won't think that we're all good because the channel isn't null.
I'm easily able to repro this on a cluster with a 1s read timeout along
with ITBLL.
Change-Id: I486fb4c099a9f110f17b4bc72e48d91f258ee044
---
M java/kudu-client/src/main/java/org/kududb/client/TabletClient.java
1 file changed, 1 insertion(+), 4 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/24/3724/1
--
To view, visit http://gerrit.cloudera.org:8080/3724
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: I486fb4c099a9f110f17b4bc72e48d91f258ee044
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Jean-Daniel Cryans <jd...@apache.org>
[kudu-CR] [java client] Fix a race involving the read timeout
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 involving the read timeout
......................................................................
Patch Set 1:
> (1 comment)
>
> It would be nice to see a test that hits this too. Generally
> speaking, the main advantage the C++ client has over the Java
> client is stress tests, many of which have revealed races and the
> like.
>
> Could we write a Java-based stress test? Or adapt ITBLL to run
> against a mini-cluster? If not, what exactly is preventing us from
> doing that?
I was hoping that ITClient would be that test, but it hasn't really found any issues as of yet. The problems I've seen require a lot of concurrency.
--
To view, visit http://gerrit.cloudera.org:8080/3724
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I486fb4c099a9f110f17b4bc72e48d91f258ee044
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: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: No
[kudu-CR] [java client] Fix a race involving the read timeout
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change.
Change subject: [java client] Fix a race involving the read timeout
......................................................................
Patch Set 2: Code-Review+2
> > (1 comment)
> >
> > It would be nice to see a test that hits this too. Generally
> > speaking, the main advantage the C++ client has over the Java
> > client is stress tests, many of which have revealed races and the
> > like.
> >
> > Could we write a Java-based stress test? Or adapt ITBLL to run
> > against a mini-cluster? If not, what exactly is preventing us
> from
> > doing that?
>
> I was hoping that ITClient would be that test, but it hasn't really
> found any issues as of yet. The problems I've seen require a lot of
> concurrency.
I don't think ITClient generates enough load. Some of the C++ tests are super aggressive in how many operations they throw at the cluster.
--
To view, visit http://gerrit.cloudera.org:8080/3724
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I486fb4c099a9f110f17b4bc72e48d91f258ee044
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: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: No
[kudu-CR] [java client] Fix a race involving the read timeout
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] Fix a race involving the read timeout
......................................................................
[java client] Fix a race involving the read timeout
The ReadTimeoutHandler isn't something we really want (per-RPC timer
would be better than socket read timeout), but some handling that was
added a year ago (01698c3) made things worse. Being the author of that other
patch, I think my fix was misguided.
The race this patch fixes is that some RPCs were able send their payload
on the wire even after cleanup() had run, but before `chan` was set to
null, when a ReadTimeoutException was fired. The proper way is to close
the channel first, then start cleaning up, so that concurrent threads who
process RPCs won't think that we're all good because the channel isn't null.
I'm easily able to repro this on a cluster with a 1s read timeout along
with ITBLL.
Change-Id: I486fb4c099a9f110f17b4bc72e48d91f258ee044
Reviewed-on: http://gerrit.cloudera.org:8080/3724
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, 1 insertion(+), 4 deletions(-)
Approvals:
Adar Dembo: Looks good to me, approved
Kudu Jenkins: Verified
--
To view, visit http://gerrit.cloudera.org:8080/3724
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: merged
Gerrit-Change-Id: I486fb4c099a9f110f17b4bc72e48d91f258ee044
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: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
[kudu-CR] [java client] Fix a race involving the read timeout
Posted by "Kudu Jenkins (Code Review)" <ge...@cloudera.org>.
Kudu Jenkins has posted comments on this change.
Change subject: [java client] Fix a race involving the read timeout
......................................................................
Patch Set 1:
Build Started http://104.196.14.100/job/kudu-gerrit/2619/
--
To view, visit http://gerrit.cloudera.org:8080/3724
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I486fb4c099a9f110f17b4bc72e48d91f258ee044
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: No
[kudu-CR] [java client] Fix a race involving the read timeout
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 involving the read timeout
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/3724/1//COMMIT_MSG
Commit Message:
Line 9: The ReadTimeoutHanlder isn't something we really want (per-RPC timer
> Nit: ReadTimeoutHandler
Done
--
To view, visit http://gerrit.cloudera.org:8080/3724
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I486fb4c099a9f110f17b4bc72e48d91f258ee044
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: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: Yes
[kudu-CR] [java client] Fix a race involving the read timeout
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change.
Change subject: [java client] Fix a race involving the read timeout
......................................................................
Patch Set 1:
(1 comment)
It would be nice to see a test that hits this too. Generally speaking, the main advantage the C++ client has over the Java client is stress tests, many of which have revealed races and the like.
Could we write a Java-based stress test? Or adapt ITBLL to run against a mini-cluster? If not, what exactly is preventing us from doing that?
http://gerrit.cloudera.org:8080/#/c/3724/1//COMMIT_MSG
Commit Message:
Line 9: The ReadTimeoutHanlder isn't something we really want (per-RPC timer
Nit: ReadTimeoutHandler
--
To view, visit http://gerrit.cloudera.org:8080/3724
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I486fb4c099a9f110f17b4bc72e48d91f258ee044
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: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: Yes
[kudu-CR] [java client] Fix a race involving the read timeout
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/3724
to look at the new patch set (#2).
Change subject: [java client] Fix a race involving the read timeout
......................................................................
[java client] Fix a race involving the read timeout
The ReadTimeoutHandler isn't something we really want (per-RPC timer
would be better than socket read timeout), but some handling that was
added a year ago (01698c3) made things worse. Being the author of that other
patch, I think my fix was misguided.
The race this patch fixes is that some RPCs were able send their payload
on the wire even after cleanup() had run, but before `chan` was set to
null, when a ReadTimeoutException was fired. The proper way is to close
the channel first, then start cleaning up, so that concurrent threads who
process RPCs won't think that we're all good because the channel isn't null.
I'm easily able to repro this on a cluster with a 1s read timeout along
with ITBLL.
Change-Id: I486fb4c099a9f110f17b4bc72e48d91f258ee044
---
M java/kudu-client/src/main/java/org/kududb/client/TabletClient.java
1 file changed, 1 insertion(+), 4 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/24/3724/2
--
To view, visit http://gerrit.cloudera.org:8080/3724
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I486fb4c099a9f110f17b4bc72e48d91f258ee044
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: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
[kudu-CR] [java client] Fix a race involving the read timeout
Posted by "Kudu Jenkins (Code Review)" <ge...@cloudera.org>.
Kudu Jenkins has posted comments on this change.
Change subject: [java client] Fix a race involving the read timeout
......................................................................
Patch Set 2:
Build Started http://104.196.14.100/job/kudu-gerrit/2620/
--
To view, visit http://gerrit.cloudera.org:8080/3724
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I486fb4c099a9f110f17b4bc72e48d91f258ee044
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: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: No