impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Bharath Vissapragada (Code Review)" <ger...@cloudera.org>
Subject [Impala-ASF-CR] IMPALA-5184: build fe against both Hive 1 & 2 APIs
Date Mon, 10 Apr 2017 20:04:46 GMT
Bharath Vissapragada has posted comments on this change.

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


Patch Set 7:

(9 comments)

http://gerrit.cloudera.org:8080/#/c/5538/7//COMMIT_MSG
Commit Message:

Line 9: This adds a compatibility shim layer with Hive 1 and Hive 2
Is there a Hive doc summarizing the compatibility changes. Did a quick web search, couldn't
find any.


PS7, Line 16: the
incomplete


http://gerrit.cloudera.org:8080/#/c/5538/7/bin/impala-config.sh
File bin/impala-config.sh:

Line 134: export IMPALA_HIVE_MAJOR_VERSION=1
This can be inferred from IMPALA_HIVE_VERSION given the naming scheme is followed? (Unless
we want to support a combination of different IMPALA_HIVE_MAJOR_VERSION and IMPALA_HIVE_VERSION,
which I don't think is the case)


http://gerrit.cloudera.org:8080/#/c/5538/7/common/thrift/.gitignore
File common/thrift/.gitignore:

Line 4: hive-2-api/TCLIService.thrift
May be add hive-*-api/TCLIService.thrift (Not sure the exact wildcard syntax works for .gitignore)?
I don't think we are expected to change Hive's spec anyway.


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

Line 189: # The thrift-generated java classes defined in TCLIService are also downloaded via
maven.
Wondering why this logic is required instead of adding a copy of hive-2-api/TCLIService.thrift
from hive-2's git repo just like how we maintain a copy of hive-1-api/....thrift. Whoever
is compiling can just toggle IMPALA_HIVE_MAJOR_VERSION and use the appropriate thrift file.
Am I missing something?


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

PS7, Line 46: MetaStoreCompatShim
MetaStoreCompatShim sounds kinda redundant, maybe just MetaStoreShim? I'm not too strong about
this, up to you :).


PS7, Line 68:           
4 space indentation, here and multiple places in this file.


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

Line 43:  * A wrapper around some of Hive's metastore API's to abstract away differences
May be add the versions this shim is compatible for? We can get it from the package name,
but I think it is worthy of a class comment (Here and the other shim too).


Line 100:   public static TResultSet execGetFunctions(Frontend frontend,
Do these belong here? They look similar across both the Shim classes.


-- 
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: 7
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong <tarmstrong@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bharathv@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message