impala-reviews mailing list archives

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

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


Patch Set 7:

(11 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 
I don't believe so. I looked at the history of some of these changes and couldn't find an
overall list.


PS7, Line 16: the
> incomplete
Done


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 fo
Yeah we probably don't need the flexibility.


Line 377: export HIVE_SRC_DIR=${HIVE_SRC_DIR:-"${HIVE_HOME}/src"}
I ran into the dreaded sticky config variable problem with some of these variables switching
between versions. This seems like a good time to fix it before more Impala devs end up switching
between versions.


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 synta
The other one is checked in, so any changes to it shouldn't be ignored.

Hive's TCLIService has had various additions to it, so it may make sense at some point to
refresh this.


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 adds additional RPC endpoints that we don't implement.


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 n
Done


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


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
Done


Line 100:   public static TResultSet execGetFunctions(Frontend frontend,
> Do these belong here? They look similar across both the Shim classes.
The thrift classes moved between packages. The code is identical but the classes moved from
the cli to rpc package so the imports up the top of the file are different. There isn't really
a good way to do conditional imports in Java.


http://gerrit.cloudera.org:8080/#/c/5538/3/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java
File fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java:

Line 3092:         }
Longline


-- 
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-Reviewer: Tim Armstrong <tarmstrong@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message