impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Henry Robinson (Code Review)" <>
Subject [Impala-ASF-CR] IMPALA-5394: Set socket timeouts while opening TSaslTransport
Date Mon, 05 Jun 2017 23:35:20 GMT
Henry Robinson has posted comments on this change.

Change subject: IMPALA-5394: Set socket timeouts while opening TSaslTransport

Patch Set 2:

(1 comment)
File be/src/transport/TSaslServerTransport.cpp:

PS2, Line 158: socket->setRecvTimeout(0);
             :     socket->setSendTimeout(0);
> I initially planned to do that, but TSocket doesn't provide a way to get th
Ah, that's what I was missing, thanks John. Still trying to make sure I understand the issue
- maybe it would better to give up transportMap_mutex_ if the if(..) branch is taken? So you'd
have something like:
    lock_guard<mutex> l(transportMap_mutex_);
    // ....
    if (trans_map != transportMap_.end()) {
      // return transport and erase entry in map

  boost::shared_ptr<TTransport> wrapped(
        new TSaslServerTransport(serverDefinitionMap_, trans));
    ret_transport.reset(new TBufferedTransport(wrapped, impala::ThriftServer::BufferedTransportFactory::DEFAULT_BUFFER_SIZE_BYTES));
    lock_guard<mutex> l(transportMap_mutex_);
    transportMap_[trans] = ret_transport;   

The most obvious problem with that is that concurrent calls to getTransport() with the same
'trans' object would race. There are ways to address that but it looks like it's assumed the
callers only use 'trans' consecutively, not concurrently.

To view, visit
To unsubscribe, visit

Gerrit-MessageType: comment
Gerrit-Change-Id: I56a5f3d9cf931cff14eae7f236fea018236a6255
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: John Sherman <>
Gerrit-Reviewer: Henry Robinson <>
Gerrit-Reviewer: John Sherman <>
Gerrit-Reviewer: Matthew Jacobs <>
Gerrit-Reviewer: Sailesh Mukil <>
Gerrit-HasComments: Yes

View raw message