Mailing-List: contact reviews-help@impala.incubator.apache.org; run by ezmlm
Precedence: bulk
Message-Id: <201706260141.v5Q1fg2i026783@ip-10-146-233-104.ec2.internal>
Date: Mon, 26 Jun 2017 01:41:41 +0000
From: "Sailesh Mukil (Code Review)" <gerrit@cloudera.org>
To: Michael Ho <kwho@cloudera.com>, impala-cr@cloudera.com,
        reviews@impala.incubator.apache.org
CC: Juan Yu <jyu@cloudera.com>, Lars Volker <lv@cloudera.com>,
        Dan Hecht <dhecht@cloudera.com>,
        Mostafa Mokhtar <mmokhtar@cloudera.com>,
        Matthew Jacobs <mj@cloudera.com>
Reply-To: sailesh@cloudera.com
Subject: =?UTF-8?Q?=5BImpala-ASF-CR=5D_IMPALA-5558/IMPALA-5576=3A_Reopen_stale_client_connection=0A?=
In-Reply-To: <gerrit.1498256993000.I4d722c8ad3bf0e78e89887b6cb286c69ca61b8f5@gerrit.cloudera.org>
References: <gerrit.1498256993000.I4d722c8ad3bf0e78e89887b6cb286c69ca61b8f5@gerrit.cloudera.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Content-Disposition: inline
User-Agent: Gerrit/2.12.7
archived-at: Mon, 26 Jun 2017 01:41:49 -0000

Sailesh Mukil has posted comments on this change.

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


Patch Set 10:

(4 comments)

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

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

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 http://gerrit.cloudera.org:8080/7284
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I4d722c8ad3bf0e78e89887b6cb286c69ca61b8f5
Gerrit-PatchSet: 10
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