Mailing-List: contact dev-help@impala.incubator.apache.org; run by ezmlm
Precedence: bulk
Reply-To: dev@impala.incubator.apache.org
Message-Id: <201604010642.u316gHpk018605@ip-10-146-233-104.ec2.internal>
Date: Fri, 1 Apr 2016 06:42:16 +0000
From: "Wes McKinney (Code Review)" <gerrit@cloudera.org>
To: Thomas Tauber-Marshall <tmarshall@cloudera.com>, impala-cr@cloudera.com,
        dev@impala.incubator.apache.org
CC: Matthew Jacobs <mj@cloudera.com>, Marcel Kornacker <marcel@cloudera.com>
Reply-To: wes@cloudera.com
Subject: 
 =?UTF-8?Q?[hs2client-CR]_Initial_structure_for_the_C++_hiveserver2_client.=0A?=
In-Reply-To: 
 <gerrit.1459192143000.I7be82d5897ad6093116915b17924674c4ae508a0@gerrit.cloudera.org>
References: 
 <gerrit.1459192143000.I7be82d5897ad6093116915b17924674c4ae508a0@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.10-rc0

Wes McKinney has posted comments on this change.

Change subject: Initial structure for the C++ hiveserver2 client.
......................................................................


Patch Set 4:

(28 comments)

http://gerrit.cloudera.org:8080/#/c/2645/4/src/hs2client/columnar-row-set.h
File src/hs2client/columnar-row-set.h:

Line 27:   const void* nulls;
`const uint8_t*` here


Line 30:     char c = (*((std::string*)nulls))[i / 8];
if you use `const uint8_t*` above, this is not needed. I don't think it's a good idea to use std::string in this manner anyhow (you may accidentally invoke constructor calls in some cases)


http://gerrit.cloudera.org:8080/#/c/2645/4/src/hs2client/hs2client.cc
File src/hs2client/hs2client.cc:

Line 49: using apache::hive::service::cli::thrift::TStatusCode;
You could instead use a namespace alias:

namespace hs2 = apache::hive::service::cli::thrift;


Line 58: using namespace std;
Better to use qualified names


Line 103: TFetchOrientation::type FetchOrientationToTFetchOrientation(
Methods that are not exported should be static


Line 132:     unique_ptr<HS2Service>* out) {
Need an HS2 protocol argument. We will eventually need to support non-columnar protocols (pre-V6). If the user passes < V6 protocol, return error Status


Line 180:   }
It may be worth creating a macro like RETURN_NOT_OK but that captures and translates Thrift errors to hs2client::Status


Line 194:   TProtocolVersion hs2_protocol_version_;
Struct data members do not have trailing underscores (https://google.github.io/styleguide/cppguide.html#Variable_Names). You may make this a class in the interest of readability


Line 198:     : impl_(new HS2SessionImpl()), rpc_(move(rpc)), open_(false) {}
std::move is not necessary with std::shared_ptr


Line 208:   rpc_->client_->CloseSession(resp, req);
Can this fail?


Line 218:   rpc_->client_->OpenSession(resp, req);
Can this fail?


Line 220:   Status status = TStatusToStatus(resp.status);
RETURN_NOT_OK(TStatusToStatus(resp.status));


Line 231:   ExecuteStatementOperation(shared_ptr<ThriftRPC> rpc) : Operation(move(rpc)) {}
const std::shared_ptr<ThriftRPC>&, and no std::move


Line 233:   Status Open(TSessionHandle sessionHandle, const string& statement,
session_handle


Line 241:     rpc_->client_->ExecuteStatement(resp, req);
failure possible?


Line 255:     const HS2ClientConfig& confOverlay, shared_ptr<Operation>* out) {
conf_overlay


Line 263:   GetTablesOperation(shared_ptr<ThriftRPC> rpc) : Operation(move(rpc)) {}
const sp<...>& and no std::move also


Line 266:       const std::string& tableName) {
Please use Google C++ naming conventions


Line 273:     rpc_->client_->GetTables(resp, req);
can this fail?


Line 306:   rpc_->client_->CancelOperation(resp, req);
handle exception thrown


Line 314:   rpc_->client_->CloseOperation(resp, req);
may throw exception


Line 330:   rpc_->client_->FetchResults(resp, req);
may throw exception


Line 331:   Status status = TStatusToStatus(resp.status);
RETURN_NOT_OK(...) ?


Line 348:   impl->column_ = row_set->columns[i];
I believe this performs a copy of the column in the row set, which must be avoided at all costs


http://gerrit.cloudera.org:8080/#/c/2645/4/src/hs2client/hs2session.h
File src/hs2client/hs2session.h:

Line 61:   Status GetFunctions(const std::string& schemaName, std::shared_ptr<Operation>* out);
May prefer C++-style variable naming in this methods. Can you add documentation to explain what `schema name` means (i.e. schemas are databases here)?


http://gerrit.cloudera.org:8080/#/c/2645/4/src/hs2client/operation.h
File src/hs2client/operation.h:

Line 46:   static const int DEFAULT_MAX_ROWS;
Is this intended to be configurable? If yes, state if the user is expected to modify Operation::DEFAULT_MAX_ROWS directly. If no, only declare this in the compilation unit.


Line 52:   Operation& operator=(Operation const&) = delete;
You may want to pull in the gutil DISALLOW_COPY_AND_ASSIGN https://github.com/apache/arrow/blob/master/cpp/src/arrow/util/macros.h#L22 (and put this in private: )


http://gerrit.cloudera.org:8080/#/c/2645/4/src/hs2client/sample-usage.cc
File src/hs2client/sample-usage.cc:

Line 103:   service->Close();
Is there concern about "zombie sessions"? We might be able to apply RAII-style to automatically close sessions in the event of exceptions / errors


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I7be82d5897ad6093116915b17924674c4ae508a0
Gerrit-PatchSet: 4
Gerrit-Project: hs2client
Gerrit-Branch: master
Gerrit-Owner: Thomas Tauber-Marshall <tmarshall@cloudera.com>
Gerrit-Reviewer: Marcel Kornacker <marcel@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tmarshall@cloudera.com>
Gerrit-Reviewer: Wes McKinney <wes@cloudera.com>
Gerrit-HasComments: Yes