impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Michael Ho (Code Review)" <>
Subject [Impala-ASF-CR] IMPALA-5558/IMPALA-5576: Reopen stale client connection
Date Sun, 25 Jun 2017 18:02:52 GMT
Michael Ho has posted comments on this change.

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

Patch Set 8:


Answering some of the questions first. Will update PS to address other comments.
File be/src/rpc/

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 thri
It comes from the following line in the thrift library:

It can happen only if read() returns 0. As you can see in the code posted above, it's using
uint32_t for the variable 'get' so it can never be < 0. So, that means it's triggered if
read() returns 0. Looking at TSocket.cpp, it corresponds to the following line:

which mean recv() returns 0.

The way readAll() is structured suggests that we should have reading non-zero length payload.
So one can only conclude based on the following line in the man page of recv():

When a stream socket peer has performed an orderly shutdown, the return value will be 0 (the
traditional "end-of-file" return).
File be/src/runtime/

PS8, Line 236: res.status
> now that we try o get the connection inside the loop, this could be uniniti
Yes, the default constructor for TStatus sets the error code to 0 (TErroCode::OK). May be
it's safer to explicitly initialize it.

Line 244:     Cancel();
> this Cancel() still results in IMPALA-5576, no? I'm not saying we should do
This is the same behavior as in previous releases. We used to cancel the fragments if we failed
the RPC:

To view, visit
To unsubscribe, visit

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