impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Sailesh Mukil (Code Review)" <>
Subject [Impala-ASF-CR] IMPALA-5558/IMPALA-5576: Reopen stale client connection
Date Mon, 26 Jun 2017 01:41:41 GMT
Sailesh Mukil has posted comments on this change.

Change subject: IMPALA-5558/IMPALA-5576: Reopen stale client connection

Patch Set 10:


Have one final question about the sleeping holding the coordinator lock. Will +1 after we
come to a conclusion on that.
File be/src/runtime/

PS10, Line 248:     if (instance_stats->done_) continue;
It would be good to mention this fix in the commit message too.
i.e. previously we would drop some other finstances statuses if we received duplicate 'done'
statuses from the same finstance(s).

PS10, Line 353: SleepForMs(100);
Just wondering if it's a good idea to have the coordinator thread go to sleep. It may hold
up a lot of work even though it's just for 300ms, since it takes the coordinator lock_ before
calling this in Coordinator::Cancel().

Is there a risk in leaving it as it was before, i.e. try to cancel without a sleep?

PS10, Line 356: status_.MergeStatus(client_status);
Isn't this redundant if you're adding its message as a detail below in L361 anyway?

PS10, Line 365: status_.MergeStatus(rpc_status);
Same here

To view, visit
To unsubscribe, visit

Gerrit-MessageType: comment
Gerrit-Change-Id: I4d722c8ad3bf0e78e89887b6cb286c69ca61b8f5
Gerrit-PatchSet: 10
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Michael Ho <>
Gerrit-Reviewer: Dan Hecht <>
Gerrit-Reviewer: Juan Yu <>
Gerrit-Reviewer: Lars Volker <>
Gerrit-Reviewer: Matthew Jacobs <>
Gerrit-Reviewer: Michael Ho <>
Gerrit-Reviewer: Mostafa Mokhtar <>
Gerrit-Reviewer: Sailesh Mukil <>
Gerrit-HasComments: Yes

View raw message