Mailing-List: contact reviews-help@impala.incubator.apache.org; run by ezmlm
Precedence: bulk
Message-Id: <201706251622.v5PGMscj020113@ip-10-146-233-104.ec2.internal>
Date: Sun, 25 Jun 2017 16:22:54 +0000
From: "Dan Hecht (Code Review)" <gerrit@cloudera.org>
To: Michael Ho <kwho@cloudera.com>, impala-cr@cloudera.com,
        reviews@impala.incubator.apache.org
CC: Matthew Jacobs <mj@cloudera.com>, Juan Yu <jyu@cloudera.com>,
        Sailesh Mukil <sailesh@cloudera.com>, Lars Volker <lv@cloudera.com>,
        Mostafa Mokhtar <mmokhtar@cloudera.com>
Reply-To: dhecht@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: Sun, 25 Jun 2017 16:22:59 -0000

Dan Hecht has posted comments on this change.

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


Patch Set 8:

(7 comments)

http://gerrit.cloudera.org:8080/#/c/7284/8/be/src/rpc/thrift-util.cc
File be/src/rpc/thrift-util.cc:

PS8, Line 198: TTransportException::END_OF_FILE &&
             :              strstr(e.what(), "No more data to read.
how do we know that this means the connection was reset? is it because thrift would never get into this state otherwise?


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

PS8, Line 350: dummy
explain why this can be ignored.


PS8, Line 362: connect_success
i don't understand that. connect_success can be true if the previous client_status was ok yet the last rpc_status was not ok. why do we want to return true only in that case? 
why not just always return true? the return value is suppose to be if "cancellation was attempted", and it's only used for a log message.


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

PS8, Line 138: message
ReportExecStatus RPC messages
to be more specific.


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

Line 225:   // Try to send the RPC 3 times before failing. Sleep for 100ms between retries.
it'd be good to explain that retry is always safe for this rpc because the coordinator handles that explicitly.


PS8, Line 236: res.status
now that we try o get the connection inside the loop, this could be uninitialized, no? or does it get initialized to ok in TReportExecStatusResult constructor?


Line 244:     Cancel();
this Cancel() still results in IMPALA-5576, no? I'm not saying we should do anything more, though. at least we'll have tried a few times before this.


-- 
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: 8
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