impala-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Wes McKinney (Code Review)" <>
Subject [hs2client-CR] Initial structure for the C++ hiveserver2 client.
Date Fri, 06 May 2016 23:59:24 GMT
Wes McKinney has posted comments on this change.

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

Patch Set 12:


a few comments from me just in case there is clarification needed
File src/hs2client/columnar-row-set.h:

Line 62:   TypedColumn(const uint8_t* nulls, const std::vector<T>* data)
> does this need to be public?
it may help with unit testing
File src/hs2client/hs2service.h:

Line 109:   Status OpenSession(const std::string& user, const HS2ClientConfig& config,
> is 'user' sufficient as an authentication mechanism?
aside: we may want some SessionParams struct with things like user in it
File src/hs2client/hs2session.h:

Line 48:       std::shared_ptr<Operation>* operation) const;
> the class comment for Operation says it's not thread-safe, so why return th
The rationale is that the operation may be handed off to some other code. You could return
unique_ptr, but then you would have to transfer it to a shared_ptr if you wished to share
ownership. Operation might do well to become thread-safe at some point.

Line 55:   Status GetSchemas(std::shared_ptr<Operation>* operation) const;
> what exactly does this do? (what is role of operation?)
Operations provide FetchResults here, I think

Line 59:   Status GetTables(std::shared_ptr<Operation>* operation) const;
> instead of returning the results of these metadata requests as generic oper
Since the results are asynchronous, you're saying you'd want to wrap the async operation in
some other struct type with a nicer API? (rather than needing do know that you need to inspect
the result set to see the returned tables).

To view, visit
To unsubscribe, visit

Gerrit-MessageType: comment
Gerrit-Change-Id: I7be82d5897ad6093116915b17924674c4ae508a0
Gerrit-PatchSet: 12
Gerrit-Project: hs2client
Gerrit-Branch: master
Gerrit-Owner: Thomas Tauber-Marshall <>
Gerrit-Reviewer: Marcel Kornacker <>
Gerrit-Reviewer: Matthew Jacobs <>
Gerrit-Reviewer: Thomas Tauber-Marshall <>
Gerrit-Reviewer: Wes McKinney <>
Gerrit-HasComments: Yes

View raw message