impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Dan Hecht (Code Review)" <ger...@cloudera.org>
Subject [Impala-ASF-CR] IMPALA-2864: Ensure that client connections are closed after a failed Open()
Date Tue, 06 Dec 2016 21:39:05 GMT
Dan Hecht has posted comments on this change.

Change subject: IMPALA-2864: Ensure that client connections are closed after a failed Open()
......................................................................


Patch Set 2: Code-Review+2

(2 comments)

http://gerrit.cloudera.org:8080/#/c/5385/2/be/src/rpc/thrift-client.cc
File be/src/rpc/thrift-client.cc:

PS2, Line 46: socket
confusing since it talks about "socket' when the code does something to the transport.  How
about just removing this comment? It seems redundant with the C++ code.


http://gerrit.cloudera.org:8080/#/c/5385/2/be/src/rpc/thrift-client.h
File be/src/rpc/thrift-client.h:

Line 55:   /// We also ensure that a failed Open() leaves the socket in a closed state.
Is "connection" and "socket" synomymous, or not? Everwhere else here we say connection.  If
so, let's say connection.

An let's be more direct about the post condition (rather than what we do to implement that):
If Open() fails, the connection remains closed.


-- 
To view, visit http://gerrit.cloudera.org:8080/5385
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ia7e883d8224304ad13a051f595d0e8abf4f9671e
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Sailesh Mukil <sailesh@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dhecht@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message