Mailing-List: contact reviews-help@impala.incubator.apache.org; run by ezmlm
Precedence: bulk
Message-Id: <201706052335.v55NZKl6029332@ip-10-146-233-104.ec2.internal>
Date: Mon, 5 Jun 2017 23:35:20 +0000
From: "Henry Robinson (Code Review)" <gerrit@cloudera.org>
To: John Sherman <jfs@arcadiadata.com>, impala-cr@cloudera.com,
        reviews@impala.incubator.apache.org
CC: Matthew Jacobs <mj@cloudera.com>, Sailesh Mukil <sailesh@cloudera.com>
Reply-To: henry@cloudera.com
Subject: =?UTF-8?Q?=5BImpala-ASF-CR=5D_IMPALA-5394=3A_Set_socket_timeouts_while_opening_TSaslTransport=0A?=
In-Reply-To: <gerrit.1496426620000.I56a5f3d9cf931cff14eae7f236fea018236a6255@gerrit.cloudera.org>
References: <gerrit.1496426620000.I56a5f3d9cf931cff14eae7f236fea018236a6255@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: Mon, 05 Jun 2017 23:35:26 -0000

Henry Robinson has posted comments on this change.

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


Patch Set 2:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/7061/2/be/src/transport/TSaslServerTransport.cpp
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));
  ret_transport.get()->open();
    
  {
    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 http://gerrit.cloudera.org:8080/7061
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I56a5f3d9cf931cff14eae7f236fea018236a6255
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: John Sherman <jfs@arcadiadata.com>
Gerrit-Reviewer: Henry Robinson <henry@cloudera.com>
Gerrit-Reviewer: John Sherman <jfs@arcadiadata.com>
Gerrit-Reviewer: Matthew Jacobs <mj@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sailesh@cloudera.com>
Gerrit-HasComments: Yes