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] Initial structure for the C++ hiveserver2 client.
Date Wed, 27 Apr 2016 20:19:37 GMT
Matthew Jacobs has posted comments on this change.

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


Patch Set 8:

(27 comments)

http://gerrit.cloudera.org:8080/#/c/2645/8//COMMIT_MSG
Commit Message:

Line 16: PIMPL-ing
super nit: but can you refer to this as "The PIMPL idiom"


Line 16: header
public header


Line 22: stored in
technically "accessible via"


Line 23: s
not plural?


Line 23: via
hm maybe too many "via"s now, can use "by"


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

Line 67: Close();
Unless there's a good reason to keep Reconnect I think it's just safer to take it out. If
we keep it here and Close() fails, the return is never handled or logged. If the client implements
this logic themselves they can decide if they want to log it or handle it somehow.


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

Line 67: The
Let's start with what this does before going into ownership (which is important too, of course).
Please prefix this with a sentence like "Creates a new connection to a HS2 service."


Line 72: out
Can you call this "service"?


Line 81: Reconnect
Let's just remove this, it's not essential and not too clear to me when users should use this.
Or if it's very clear when/why to use it, let's document it.


Line 85:   // The client calling OpenSession has ownership of the HS2Session that is created.
Let's start with what this does before going into ownership (which is important too, of course).
Please prefix this with a sentence like "Opens a new HS2 session using this service."


Line 86: Executing RPCs with an Operation corresponding to a particular HS2Session after
       :   // that HS2Session has been closed or deleted is undefined.
I think this sentence is better suited on HS2Session. Right here it'd be good to add something
like "Operations on the HS2Session are undefined once this HS2Session is closed."


Line 89: out
session


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

Line 44: out
operation, and below as well


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

Line 36: assert
DCHECK?


Line 84: status
dcheck this is an OK status. I assume we can't just return Status::OK because this could have
a message w/ it?


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

Line 55:  * have Close called on them before they can be deleted.
Please add: This class is not thread-safe.

Also add that to the other main interface class header comments.


Line 61: // May be called after successfully creating the operation and before calling Close.
The comment here and for all the public API functions should include a brief description of
what these do, parameters, return values. However, I'd also like to get the initial code in
soon so I'm OK with a follow-up review to go through and comment more thoroughly. For example,
here's how I'd document this function:

 
// Fetches the current state of this operation. If successful, sets the
// operation state in 'out' and returns an OK status, otherwise an
// error status is returned. May be called after successfully creating 
// the operation and before calling Close.


Line 71: Fetch will return success but 'out' will be empty.
fyi this means we have to be careful not to return empty row batches unless we actually get
an eos from the server side.  While it's probably not likely to happen unless something is
wrong, I don't think impala guarantees it gives non-empty result batches and we need to guard
against it. We could handle it by continuing to fetch until something comes, or at least we
should probably have a DCHECK to verify.


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

Line 83: string
const string&


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

Line 38: Status
This should be commented as well. Let's do it in a follow-up review.


http://gerrit.cloudera.org:8080/#/c/2645/8/src/hs2client/test-util.h
File src/hs2client/test-util.h:

Line 109:     session_->Close();
        :     service_->Close();
You need to make sure these are always called. the EXPECT_OKs above will return early.


http://gerrit.cloudera.org:8080/#/c/2645/8/src/hs2client/thrift-internal.cc
File src/hs2client/thrift-internal.cc:

Line 31: unkown
typo, also append the value to the string for debugging purposes


Line 50: default:
This is converting from our types to hs2 types, so we shouldn't have undefined cases. Add
a DCHECK we don't get here before returning


Line 71: default
same dcheck


Line 95: default:
Let's log if this happens, including the value of tstate


Line 120: Unkown
typo


http://gerrit.cloudera.org:8080/#/c/2645/8/src/hs2client/thrift-internal.h
File src/hs2client/thrift-internal.h:

Line 70: #define RETURN_NOT_OK(tstatus)                                            \
       :   if (tstatus.statusCode != hs2::TStatusCode::SUCCESS_STATUS &&         
 \
       :       tstatus.statusCode != hs2::TStatusCode::SUCCESS_WITH_INFO_STATUS) { \
       :     return TStatusToStatus(tstatus);                                      \
       :   }
       : 
       : #define RETURN_MSG_NOT_OK(tstatus)                                        \
       :   if (tstatus.statusCode != hs2::TStatusCode::SUCCESS_STATUS &&         
 \
       :       tstatus.statusCode != hs2::TStatusCode::SUCCESS_WITH_INFO_STATUS) { \
       :     return TStatusToStatus(tstatus).GetMessage();                         \
       :   }
I don't see these being used and I don't think they're safely written macros. Can you remove
them? If you need macros to do this we need to wrap them in a do/while loop or so.


-- 
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: 8
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

Mime
View raw message