Mailing-List: contact reviews-help@impala.incubator.apache.org; run by ezmlm
Precedence: bulk
Message-Id: <201704172341.v3HNfoSg010415@ip-10-146-233-104.ec2.internal>
Date: Mon, 17 Apr 2017 23:41:50 +0000
From: "Tim Armstrong (Code Review)" <gerrit@cloudera.org>
To: impala-cr@cloudera.com, reviews@impala.incubator.apache.org
CC: Alex Behm <alex.behm@cloudera.com>,
        Bharath Vissapragada <bharathv@cloudera.com>
Reply-To: tarmstrong@cloudera.com
Subject: =?UTF-8?Q?=5BImpala-ASF-CR=5D_IMPALA-5184=3A_build_fe_against_both_Hive_1_=26_2_APIs=0A?=
In-Reply-To: <gerrit.1481935488000.Ifbc265281c04fe3136bc3c920dbac966742ce09a@gerrit.cloudera.org>
References: <gerrit.1481935488000.Ifbc265281c04fe3136bc3c920dbac966742ce09a@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.12.7
archived-at: Mon, 17 Apr 2017 23:41:54 -0000

Tim Armstrong has posted comments on this change.

Change subject: IMPALA-5184: build fe against both Hive 1 & 2 APIs
......................................................................


Patch Set 9:

(8 comments)

http://gerrit.cloudera.org:8080/#/c/5538/9/common/thrift/CMakeLists.txt
File common/thrift/CMakeLists.txt:

Line 189: # The thrift-generated java classes defined in TCLIService are also downloaded via maven.
> Can you expand the first sentence to explain in more detail why the downloa
Done.


http://gerrit.cloudera.org:8080/#/c/5538/9/fe/src/compat-hive-1/java/org/apache/impala/compat/MetaStoreShim.java
File fe/src/compat-hive-1/java/org/apache/impala/compat/MetaStoreShim.java:

Line 34: import org.apache.impala.authorization.User;
> Why is there Impala stuff in here if this is a shim for the Hive Metastore?
The problem is that the implementations of execGetThing() depend on declaring variables with types like TGetThingReq. To declare the variable, we need to import the class, and to import the class we need to name the package that the class is in. The last part is the problem.


Line 44:  * A wrapper around some of Hive's metastore API's to abstract away differences
> Metastore
Done


Line 45:  * between major versions of hive. This implements the shimmed methods for Hive 2.
> Hive
Done


Line 47: public class MetaStoreShim {
> Let's use "Metastore" and not "MetaStore". Impala uses the former consisten
Done


http://gerrit.cloudera.org:8080/#/c/5538/9/fe/src/compat-hive-2/java/org/apache/impala/compat/MetaStoreShim.java
File fe/src/compat-hive-2/java/org/apache/impala/compat/MetaStoreShim.java:

Line 44:  * A wrapper around some of Hive's metastore API's to abstract away differences
> Metastore
Done


Line 45:  * between major versions of hive. This implements the shimmed methods for Hive 2.
> Hive
Done


PS9, Line 100: p
> This and the following functions look identical between the shim implementa
The imports up the top of the file for TGetThingReq are different unfortunately. This is a workaround for the fact that there's no good way to do any kind of conditional important.


-- 
To view, visit http://gerrit.cloudera.org:8080/5538
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ifbc265281c04fe3136bc3c920dbac966742ce09a
Gerrit-PatchSet: 9
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong <tarmstrong@cloudera.com>
Gerrit-Reviewer: Alex Behm <alex.behm@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bharathv@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <tarmstrong@cloudera.com>
Gerrit-HasComments: Yes