impala-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Matthew Jacobs (Code Review)" <ger...@cloudera.org>
Subject [hs2client-CR] Implemented a framework for the C++ hiveserver2 client.
Date Tue, 29 Mar 2016 01:21:35 GMT
Matthew Jacobs has posted comments on this change.

Change subject: Implemented a framework for the C++ hiveserver2 client.
......................................................................


Patch Set 2:

(14 comments)

I think the interface is getting pretty close, but it's hard to sign off on just the interface
when I think some things may change as you discover small things that come up in the implementation.
I think it's ready to start moving forward with the implementation. Let's see if we can get
a proof-of-concept plumbing an ExecStatement all the way through?

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

can you use dashes between words in filenames instead of underscores to be consistent with
impala c++ file naming?


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

Line 21: #include "gen-cpp/TCLIService.h"
we typically have includes ordered like this:

 
#include <this_header>
<blank line>
#include <ext libraries>
<blank line>
#include "impala headers"
<blank line>
#include "impala generated headers (i.e. gen-cpp/*"


Line 30: using namespace std;
nit: line above 30


Line 46: 
nit remove extra space


Line 54: HS2Service::~HS2Service() = default;
Can you implement Close() & Reconnect()? Then this would be a good place to put DCHECKs
that ensure the state is already cleaned up properly.


Line 61: 
nit: remove these extra lines? I don't think they increase readability here (or push back
if you think they do :) )


Line 70: 
nit: rm extra line


http://gerrit.cloudera.org:8080/#/c/2645/2/src/hs2client/hs2service.h
File src/hs2client/hs2service.h:

Line 32: string
const std::string&


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

Line 27: default
we should also figure out if we can check everything is cleaned up properly by the time this
is destructed i.e. that Close() was called if it needs to be


http://gerrit.cloudera.org:8080/#/c/2645/2/src/hs2client/status.h
File src/hs2client/status.h:

Line 40: SuccessWithInfo
Do we really need this?


Line 41: StillExecuting
what does this mean?


Line 46: IsSuccessWithInfo
Let's see if we can get rid of this, it seems like a caller might have to check both IsSuccess
and IsSuccessWithInfo to find out if the rpc worked


Line 51: ok
overlap with the IsSucesses?


Line 54: string
is this returning internal state (e.g. a member that isn't on this class yet) or a string
that this creates on the stack?


-- 
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: 2
Gerrit-Project: hs2client
Gerrit-Branch: master
Gerrit-Owner: Thomas Tauber-Marshall <tmarshall@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message