You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@impala.apache.org by "Henry Robinson (Code Review)" <ge...@cloudera.org> on 2016/06/06 19:28:57 UTC
[Impala-CR](cdh5-trunk) IMPALA-3682: Don't retry unrecoverable socket creation errors
Henry Robinson has uploaded a new change for review.
http://gerrit.cloudera.org:8080/3317
Change subject: IMPALA-3682: Don't retry unrecoverable socket creation errors
......................................................................
IMPALA-3682: Don't retry unrecoverable socket creation errors
If a thrift client can't create a socket, all subsequent calls to Open()
should fail fast since socket creation errors are treated as unrecoverable.
Change-Id: I394be287143eefc79cf22865898b71ca24c41328
---
M be/src/rpc/thrift-client.cc
M be/src/runtime/exec-env.cc
M common/thrift/generate_error_codes.py
3 files changed, 6 insertions(+), 3 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala refs/changes/17/3317/1
--
To view, visit http://gerrit.cloudera.org:8080/3317
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: I394be287143eefc79cf22865898b71ca24c41328
Gerrit-PatchSet: 1
Gerrit-Project: Impala
Gerrit-Branch: cdh5-trunk
Gerrit-Owner: Henry Robinson <he...@cloudera.com>
[Impala-CR](cdh5-trunk) IMPALA-3682: Don't retry unrecoverable socket creation errors
Posted by "Henry Robinson (Code Review)" <ge...@cloudera.org>.
Henry Robinson has posted comments on this change.
Change subject: IMPALA-3682: Don't retry unrecoverable socket creation errors
......................................................................
Patch Set 2:
I added a test case that exercises that path. Doesn't test this change explicitly, but confirms the correct behavior is maintained.
--
To view, visit http://gerrit.cloudera.org:8080/3317
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I394be287143eefc79cf22865898b71ca24c41328
Gerrit-PatchSet: 2
Gerrit-Project: Impala
Gerrit-Branch: cdh5-trunk
Gerrit-Owner: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-HasComments: No
[Impala-CR](cdh5-trunk) IMPALA-3682: Don't retry unrecoverable socket creation errors
Posted by "Henry Robinson (Code Review)" <ge...@cloudera.org>.
Hello Sailesh Mukil, Dan Hecht,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/3317
to look at the new patch set (#2).
Change subject: IMPALA-3682: Don't retry unrecoverable socket creation errors
......................................................................
IMPALA-3682: Don't retry unrecoverable socket creation errors
If a thrift client can't create a socket, all subsequent calls to Open()
should fail fast since socket creation errors are treated as
unrecoverable.
Testing: manual testing with a bad SSL configuration. Impalad startup
fails fast, rather than retrying 10 times as previously.
Change-Id: I394be287143eefc79cf22865898b71ca24c41328
---
M be/src/rpc/thrift-client.cc
M be/src/rpc/thrift-server-test.cc
M be/src/runtime/exec-env.cc
M common/thrift/generate_error_codes.py
4 files changed, 56 insertions(+), 19 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala refs/changes/17/3317/2
--
To view, visit http://gerrit.cloudera.org:8080/3317
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I394be287143eefc79cf22865898b71ca24c41328
Gerrit-PatchSet: 2
Gerrit-Project: Impala
Gerrit-Branch: cdh5-trunk
Gerrit-Owner: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
[Impala-CR](cdh5-trunk) IMPALA-3682: Don't retry unrecoverable socket creation errors
Posted by "Henry Robinson (Code Review)" <ge...@cloudera.org>.
Henry Robinson has posted comments on this change.
Change subject: IMPALA-3682: Don't retry unrecoverable socket creation errors
......................................................................
Patch Set 1:
Hard to add automated testing, partly because the eventual behavior hasn't changed, just the number of retries it takes to get there. I did manually test this with a bad SSL setup, and saw a fast failure as expected. I've added this to the commit message.
--
To view, visit http://gerrit.cloudera.org:8080/3317
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I394be287143eefc79cf22865898b71ca24c41328
Gerrit-PatchSet: 1
Gerrit-Project: Impala
Gerrit-Branch: cdh5-trunk
Gerrit-Owner: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-HasComments: No
[Impala-CR](cdh5-trunk) IMPALA-3682: Don't retry unrecoverable socket creation errors
Posted by "Dan Hecht (Code Review)" <ge...@cloudera.org>.
Dan Hecht has posted comments on this change.
Change subject: IMPALA-3682: Don't retry unrecoverable socket creation errors
......................................................................
Patch Set 1:
> Hard to add automated testing, partly because the eventual behavior
> hasn't changed, just the number of retries it takes to get there. I
> did manually test this with a bad SSL setup, and saw a fast failure
> as expected. I've added this to the commit message.
Yeah, I can see why testing the old vs new is tough, but do we have an automated test to exercise this path?
--
To view, visit http://gerrit.cloudera.org:8080/3317
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I394be287143eefc79cf22865898b71ca24c41328
Gerrit-PatchSet: 1
Gerrit-Project: Impala
Gerrit-Branch: cdh5-trunk
Gerrit-Owner: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-HasComments: No
[Impala-CR](cdh5-trunk) IMPALA-3682: Don't retry unrecoverable socket creation errors
Posted by "Henry Robinson (Code Review)" <ge...@cloudera.org>.
Henry Robinson has submitted this change and it was merged.
Change subject: IMPALA-3682: Don't retry unrecoverable socket creation errors
......................................................................
IMPALA-3682: Don't retry unrecoverable socket creation errors
If a thrift client can't create a socket, all subsequent calls to Open()
should fail fast since socket creation errors are treated as
unrecoverable.
Testing: manual testing with a bad SSL configuration. Impalad startup
fails fast, rather than retrying 10 times as previously.
Change-Id: I394be287143eefc79cf22865898b71ca24c41328
Reviewed-on: http://gerrit.cloudera.org:8080/3317
Reviewed-by: Dan Hecht <dh...@cloudera.com>
Tested-by: Henry Robinson <he...@cloudera.com>
---
M be/src/rpc/thrift-client.cc
M be/src/rpc/thrift-server-test.cc
M be/src/runtime/exec-env.cc
M common/thrift/generate_error_codes.py
4 files changed, 56 insertions(+), 19 deletions(-)
Approvals:
Henry Robinson: Verified
Dan Hecht: Looks good to me, approved
--
To view, visit http://gerrit.cloudera.org:8080/3317
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: merged
Gerrit-Change-Id: I394be287143eefc79cf22865898b71ca24c41328
Gerrit-PatchSet: 3
Gerrit-Project: Impala
Gerrit-Branch: cdh5-trunk
Gerrit-Owner: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
[Impala-CR](cdh5-trunk) IMPALA-3682: Don't retry unrecoverable socket creation errors
Posted by "Henry Robinson (Code Review)" <ge...@cloudera.org>.
Henry Robinson has posted comments on this change.
Change subject: IMPALA-3682: Don't retry unrecoverable socket creation errors
......................................................................
Patch Set 2: Code-Review+1
Code build passed in this job:
http://sandbox.jenkins.cloudera.com/job/impala-umbrella-build-and-test/1726/
--
To view, visit http://gerrit.cloudera.org:8080/3317
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I394be287143eefc79cf22865898b71ca24c41328
Gerrit-PatchSet: 2
Gerrit-Project: Impala
Gerrit-Branch: cdh5-trunk
Gerrit-Owner: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-HasComments: No
[Impala-CR](cdh5-trunk) IMPALA-3682: Don't retry unrecoverable socket creation errors
Posted by "Henry Robinson (Code Review)" <ge...@cloudera.org>.
Henry Robinson has posted comments on this change.
Change subject: IMPALA-3682: Don't retry unrecoverable socket creation errors
......................................................................
Patch Set 2: -Code-Review Verified+1
--
To view, visit http://gerrit.cloudera.org:8080/3317
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I394be287143eefc79cf22865898b71ca24c41328
Gerrit-PatchSet: 2
Gerrit-Project: Impala
Gerrit-Branch: cdh5-trunk
Gerrit-Owner: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-HasComments: No
[Impala-CR](cdh5-trunk) IMPALA-3682: Don't retry unrecoverable socket creation errors
Posted by "Henry Robinson (Code Review)" <ge...@cloudera.org>.
Henry Robinson has posted comments on this change.
Change subject: IMPALA-3682: Don't retry unrecoverable socket creation errors
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/3317/1/be/src/rpc/thrift-client.cc
File be/src/rpc/thrift-client.cc:
PS1, Line 36: if (!socket_create_status_.ok()) return socket_create_status_;
> Any reason to still leave this here now? I can't recall if direct calls to
Even if not, we should be defensive against the possibility that someone will try to call Open() again in the future (and if we don't think they should, we should make that private).
--
To view, visit http://gerrit.cloudera.org:8080/3317
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I394be287143eefc79cf22865898b71ca24c41328
Gerrit-PatchSet: 1
Gerrit-Project: Impala
Gerrit-Branch: cdh5-trunk
Gerrit-Owner: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-CR](cdh5-trunk) IMPALA-3682: Don't retry unrecoverable socket creation errors
Posted by "Dan Hecht (Code Review)" <ge...@cloudera.org>.
Dan Hecht has posted comments on this change.
Change subject: IMPALA-3682: Don't retry unrecoverable socket creation errors
......................................................................
Patch Set 1: Code-Review+2
Any testing we could add for this?
--
To view, visit http://gerrit.cloudera.org:8080/3317
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I394be287143eefc79cf22865898b71ca24c41328
Gerrit-PatchSet: 1
Gerrit-Project: Impala
Gerrit-Branch: cdh5-trunk
Gerrit-Owner: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-HasComments: No
[Impala-CR](cdh5-trunk) IMPALA-3682: Don't retry unrecoverable socket creation errors
Posted by "Dan Hecht (Code Review)" <ge...@cloudera.org>.
Dan Hecht has posted comments on this change.
Change subject: IMPALA-3682: Don't retry unrecoverable socket creation errors
......................................................................
Patch Set 2: Code-Review+2
Great, thanks.
--
To view, visit http://gerrit.cloudera.org:8080/3317
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I394be287143eefc79cf22865898b71ca24c41328
Gerrit-PatchSet: 2
Gerrit-Project: Impala
Gerrit-Branch: cdh5-trunk
Gerrit-Owner: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-HasComments: No
[Impala-CR](cdh5-trunk) IMPALA-3682: Don't retry unrecoverable socket creation errors
Posted by "Sailesh Mukil (Code Review)" <ge...@cloudera.org>.
Sailesh Mukil has posted comments on this change.
Change subject: IMPALA-3682: Don't retry unrecoverable socket creation errors
......................................................................
Patch Set 1: Code-Review+1
(1 comment)
http://gerrit.cloudera.org:8080/#/c/3317/1/be/src/rpc/thrift-client.cc
File be/src/rpc/thrift-client.cc:
PS1, Line 36: if (!socket_create_status_.ok()) return socket_create_status_;
Any reason to still leave this here now? I can't recall if direct calls to Open() are made.
--
To view, visit http://gerrit.cloudera.org:8080/3317
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I394be287143eefc79cf22865898b71ca24c41328
Gerrit-PatchSet: 1
Gerrit-Project: Impala
Gerrit-Branch: cdh5-trunk
Gerrit-Owner: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-CR](cdh5-trunk) IMPALA-3682: Don't retry unrecoverable socket creation errors
Posted by "Sailesh Mukil (Code Review)" <ge...@cloudera.org>.
Sailesh Mukil has posted comments on this change.
Change subject: IMPALA-3682: Don't retry unrecoverable socket creation errors
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/3317/1/be/src/rpc/thrift-client.cc
File be/src/rpc/thrift-client.cc:
PS1, Line 36: if (!socket_create_status_.ok()) return socket_create_status_;
> Even if not, we should be defensive against the possibility that someone wi
Makes sense.
--
To view, visit http://gerrit.cloudera.org:8080/3317
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I394be287143eefc79cf22865898b71ca24c41328
Gerrit-PatchSet: 1
Gerrit-Project: Impala
Gerrit-Branch: cdh5-trunk
Gerrit-Owner: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-HasComments: Yes