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 Sat, 16 Apr 2016 00:47:40 GMT
Matthew Jacobs has posted comments on this change.

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


Patch Set 3:

(1 comment)

Just responding to a comment on patch set 3. I'll review the new patch set later.

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

Line 26: CHECK_OK
> Defining macros for checking statuses like this is fairly common practice, 
Yeah, we do often use macros for things like verifying status objects but typically macros
are best to avoid unless there's a good reason to use them. RETURN_IF_ERROR() is kind of a
useful macro due to it actually returning control flow from the current function on an error
(consider the alternative).  However, something like EXIT_IF_ERROR (which is basically what
you have here) isn't really providing much beyond what a similar function is since the control
flow at the point of calling it doesn't need to branch differently. Then I think we typically
only really provide EXIT_IF_ERROR to be consistent with RETURN_IF_ERROR, but we don't have
those here so it feels out of place. If anything though, macro code is very error prone and
hard to read. Anything that compromises readability should be avoided unless there's a good
reason to do 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: 3
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