You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Dan Burkert (Code Review)" <ge...@cloudera.org> on 2017/09/05 22:41:36 UTC
[kudu-CR] KUDU-2130 (part 2): more fixes for ITClientStress
Hello Adar Dembo, Todd Lipcon, Alexey Serbin,
I'd like you to do a code review. Please visit
http://gerrit.cloudera.org:8080/7964
to review the following change.
Change subject: KUDU-2130 (part 2): more fixes for ITClientStress
......................................................................
KUDU-2130 (part 2): more fixes for ITClientStress
This fixes some more race conditions in connection termination in the
same vein as part 1. It also filters benign SSLException from being
returned back to callers.
Change-Id: Ic3f518513931c660fd93b4272d1b1fceb268f191
---
M java/kudu-client/src/main/java/org/apache/kudu/client/Connection.java
M java/kudu-client/src/test/java/org/apache/kudu/client/ITClientStress.java
2 files changed, 17 insertions(+), 8 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/64/7964/1
--
To view, visit http://gerrit.cloudera.org:8080/7964
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: Ic3f518513931c660fd93b4272d1b1fceb268f191
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] KUDU-2130 (part 2): more fixes for ITClientStress
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert has submitted this change and it was merged.
Change subject: KUDU-2130 (part 2): more fixes for ITClientStress
......................................................................
KUDU-2130 (part 2): more fixes for ITClientStress
This fixes some more race conditions in connection termination in the
same vein as part 1. It also filters benign SSLException from being
returned back to callers.
Change-Id: Ic3f518513931c660fd93b4272d1b1fceb268f191
Reviewed-on: http://gerrit.cloudera.org:8080/7964
Reviewed-by: Alexey Serbin <as...@cloudera.com>
Tested-by: Kudu Jenkins
---
M java/kudu-client/src/main/java/org/apache/kudu/client/Connection.java
M java/kudu-client/src/test/java/org/apache/kudu/client/ITClientStress.java
2 files changed, 22 insertions(+), 8 deletions(-)
Approvals:
Alexey Serbin: Looks good to me, approved
Kudu Jenkins: Verified
--
To view, visit http://gerrit.cloudera.org:8080/7964
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: merged
Gerrit-Change-Id: Ic3f518513931c660fd93b4272d1b1fceb268f191
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] KUDU-2130 (part 2): more fixes for ITClientStress
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change.
Change subject: KUDU-2130 (part 2): more fixes for ITClientStress
......................................................................
Patch Set 2: Code-Review+2
(2 comments)
http://gerrit.cloudera.org:8080/#/c/7964/1/java/kudu-client/src/main/java/org/apache/kudu/client/Connection.java
File java/kudu-client/src/main/java/org/apache/kudu/client/Connection.java:
Line 354: lock.lock();
> Yep, found the following in an ITClientStress log:
Aha, I see. Thanks!
PS1, Line 427: error = new RecoverableExceptio
> Done
Thank you for adding the explanatory comment.
--
To view, visit http://gerrit.cloudera.org:8080/7964
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic3f518513931c660fd93b4272d1b1fceb268f191
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2130 (part 2): more fixes for ITClientStress
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/7964
to look at the new patch set (#2).
Change subject: KUDU-2130 (part 2): more fixes for ITClientStress
......................................................................
KUDU-2130 (part 2): more fixes for ITClientStress
This fixes some more race conditions in connection termination in the
same vein as part 1. It also filters benign SSLException from being
returned back to callers.
Change-Id: Ic3f518513931c660fd93b4272d1b1fceb268f191
---
M java/kudu-client/src/main/java/org/apache/kudu/client/Connection.java
M java/kudu-client/src/test/java/org/apache/kudu/client/ITClientStress.java
2 files changed, 22 insertions(+), 8 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/64/7964/2
--
To view, visit http://gerrit.cloudera.org:8080/7964
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ic3f518513931c660fd93b4272d1b1fceb268f191
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert <da...@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>
[kudu-CR] KUDU-2130 (part 2): more fixes for ITClientStress
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert has posted comments on this change.
Change subject: KUDU-2130 (part 2): more fixes for ITClientStress
......................................................................
Patch Set 1:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/7964/1/java/kudu-client/src/main/java/org/apache/kudu/client/Connection.java
File java/kudu-client/src/main/java/org/apache/kudu/client/Connection.java:
Line 216: try {
> Would it make sense to add the short-circuiting
I'm not sure, but it couldn't hurt, so I'll add it.
Line 354: return;
> I'm curious -- did it happen in the wild that the message arrived in here b
Yep, found the following in an ITClientStress log:
21:47:36.158 [ERROR - New I/O worker #1119] (Connection.java:423) [peer master-127.23.143.1:64034] unexpected exception from downstream on [id: 0xb7be1b31, /127.0.0.1:55649 :> /127.23.143.1:64034]
java.lang.IllegalStateException
at com.google.common.base.Preconditions.checkState(Preconditions.java:429)
at org.apache.kudu.client.Connection.messageReceived(Connection.java:350)
at org.jboss.netty.channel.SimpleChannelUpstreamHandler.handleUpstream(SimpleChannelUpstreamHandler.java:70)
at org.apache.kudu.client.Connection.handleUpstream(Connection.java:236)
PS1, Line 427: assert !explicitlyDisconnected;
> I'm not sure it's good to have this assertion here -- this case covers not
Done
--
To view, visit http://gerrit.cloudera.org:8080/7964
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic3f518513931c660fd93b4272d1b1fceb268f191
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2130 (part 2): more fixes for ITClientStress
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change.
Change subject: KUDU-2130 (part 2): more fixes for ITClientStress
......................................................................
Patch Set 1:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/7964/1/java/kudu-client/src/main/java/org/apache/kudu/client/Connection.java
File java/kudu-client/src/main/java/org/apache/kudu/client/Connection.java:
Line 216: try {
Would it make sense to add the short-circuiting
if (state == State.TERMINATED) {
return;
}
check here as well? Or we have some guarantees from Netty that it would not be the case?
Line 354: return;
I'm curious -- did it happen in the wild that the message arrived in here but the connection has been terminated already?
PS1, Line 427: assert !explicitlyDisconnected;
I'm not sure it's good to have this assertion here -- this case covers not only SSLException case, as I can see.
Could you add a comment with reasoning behind adding this assertion if you think it's worth adding this assertion?
--
To view, visit http://gerrit.cloudera.org:8080/7964
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic3f518513931c660fd93b4272d1b1fceb268f191
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert <da...@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