Mailing-List: contact reviews-help@impala.incubator.apache.org; run by ezmlm
Precedence: bulk
Message-Id: <201706252354.v5PNsGnA024974@ip-10-146-233-104.ec2.internal>
Date: Sun, 25 Jun 2017 23:54:16 +0000
From: "Michael Ho (Code Review)" <gerrit@cloudera.org>
To: 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>,
        Dan Hecht <dhecht@cloudera.com>,
        Mostafa Mokhtar <mmokhtar@cloudera.com>
Reply-To: kwho@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 23:54:21 -0000

Michael Ho has posted comments on this change.

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


Patch Set 9:

(15 comments)

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

PS9, Line 18: to appear succeed
> nit: typo, to appear to succeed
Missed that before pushing. Will update before merging.


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.
> Can you put this in a comment as well?
Done


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

PS9, Line 200:  
> nit: extra space
Done


http://gerrit.cloudera.org:8080/#/c/7284/9/be/src/runtime/client-cache.h
File be/src/runtime/client-cache.h:

PS9, Line 243: ) {
> not suggesting we do this now, but why do we not handle recv cxn closed err
Yes, this may be worth some re-thinking. FWIW, most callers cannot handle recv side failure any way. I believe only TrasmitData() / ReportExecStatusAux() and Cancel() will handle it.


PS9, Line 301: rpc send $3 done
> nit
Done


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 362: true;
> I was also wondering if this return value isn't very useful. What if we ins
The status and the backtrace is most likely logged when it's constructed initially deep down the call stack.

I will keep the return value as-is for now to avoid more unexpected complication.


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

PS9, Line 247: duplicated
> nit: duplicate
Done


PS9, Line 248: if (instance_exec_status.done && instance_stats->done_) continue;
> Is there any reason we want to process a not done status from a fragment th
Good point. We should simply ignore it. The current check is to make sure we don't subtract num_remaining_instances_ more than once for a given fragment instance.


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

PS9, Line 138: duplicated
> nit: duplicate
Done


PS9, Line 138: is done
> can you make this more specific here? e.g. maybe "true if the final report 
Done


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

PS9, Line 227: d
> nit: duplicate
Done


http://gerrit.cloudera.org:8080/#/c/7284/9/be/src/testutil/fault-injection-util.h
File be/src/testutil/fault-injection-util.h:

PS9, Line 64:   ///
> not clear this is the mod
Rephrased the comment.


PS9, Line 66: freq
> maybe this should be every_nth_rpc or such
Rephrased the comment.


PS9, Line 75: FAULT_INJECTION_RPC_EXCEPTION
> it'd be nice to have FAULT_INJECTION_SEND_RPC_EX and FAULT_INJECTION_RECV_R
Done


http://gerrit.cloudera.org:8080/#/c/7284/9/tests/custom_cluster/test_rpc_exception.py
File tests/custom_cluster/test_rpc_exception.py:

PS9, Line 72: execute_test_query("Called read on non-open socket"
> why does this result in a query failure whereas recv_timed_out does not ?  
Most likely, in this case, TSocket was closed (potentially due to programming error in our part). In the case of timeout, the socket is still opened but it just hits the timeout we specify when waiting for data to show up.


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