Mailing-List: contact dev-help@impala.incubator.apache.org; run by ezmlm
Precedence: bulk
Reply-To: dev@impala.incubator.apache.org
Message-Id: <201604160047.u3G0lec3017857@ip-10-146-233-104.ec2.internal>
Date: Sat, 16 Apr 2016 00:47:40 +0000
From: "Matthew Jacobs (Code Review)" <gerrit@cloudera.org>
To: Thomas Tauber-Marshall <tmarshall@cloudera.com>, impala-cr@cloudera.com,
        dev@impala.incubator.apache.org
CC: Marcel Kornacker <marcel@cloudera.com>, Wes McKinney <wes@cloudera.com>
Reply-To: mj@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

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