impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Matthew Jacobs (Code Review)" <ger...@cloudera.org>
Subject [Impala-ASF-CR] IMPALA-5558: Reopen stale client connection
Date Sun, 25 Jun 2017 00:14:11 GMT
Matthew Jacobs has posted comments on this change.

Change subject: IMPALA-5558: Reopen stale client connection
......................................................................


Patch Set 7:

(5 comments)

Did a first pass, I think it makes sense but I'll need to think a bit more about it. I'll
look again with fresh eyes later or in the morning. I'm still cautious of doing this vs reverting
the prev patches, but that needs more thought as well.

http://gerrit.cloudera.org:8080/#/c/7284/7//COMMIT_MSG
Commit Message:

Line 7: IMPALA-5558: Reopen stale client connection
you can also say this fixes IMPALA-5576


PS7, Line 40: cncelled
typo


PS7, Line 36: This change also fixes QueryState::ReportExecStatusAux() to unconditionally
            : for up to 3 times when reporting exec status of a fragment instance. Previously,
            : it may break out of the loop early if RPC fails with 'retry_is_safe' == true
            : (e.g. due to stale connections above) or if the connection to coordinator fails.
            : Failing the RPC may cause all fragment instances of a query to be cncelled locally,
            : triggering query hang due to IMPALA-2990. The status reporting is idempotent
as
            : the handler simply ignores profiles of fragment instances which are done already
            : so it should be retried uncontionally up to 3 times. Similarly, the cancellation
            : RPC is also idempotent so it should be unconditionally retried up to 3 times.
nit: wrap


http://gerrit.cloudera.org:8080/#/c/7284/7/be/src/runtime/coordinator-backend-state.cc
File be/src/runtime/coordinator-backend-state.cc:

PS7, Line 347: if (i < 2) SleepForMs(100);
why?


http://gerrit.cloudera.org:8080/#/c/7284/7/be/src/runtime/query-state.cc
File be/src/runtime/query-state.cc:

PS7, Line 237: !client_status.ok()
I think that adding this here is essentially equivalent to the bug IMPALA-5576, no?


-- 
To view, visit http://gerrit.cloudera.org:8080/7284
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I4d722c8ad3bf0e78e89887b6cb286c69ca61b8f5
Gerrit-PatchSet: 7
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Michael Ho <kwho@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dhecht@cloudera.com>
Gerrit-Reviewer: Juan Yu <jyu@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj@cloudera.com>
Gerrit-Reviewer: Michael Ho <kwho@cloudera.com>
Gerrit-Reviewer: Mostafa Mokhtar <mmokhtar@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sailesh@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message