Philip Zeyliger has posted comments on this change. ( http://gerrit.cloudera.org:8080/9124 )
Change subject: IMPALA-6440: Backwards compatibility for HBase metadata ...................................................................... Patch Set 1: (4 comments) Minor feedback. (Code logic itself looks fine.) I'm a little worried about the fact that we've published instructions on how to use "hbase.table.name" and that this might complicate things. Are you planning to change "hbase.table.name" references in various dataload-related scripts? (I'd be cool with that as a follow-on.) http://gerrit.cloudera.org:8080/#/c/9124/1//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/9124/1//COMMIT_MSG@9 PS1, Line 9: Upstream Hive has changed the property name used to specify I think also HBase? Can we pull in the JIRA numbers in the corresponding projects? http://gerrit.cloudera.org:8080/#/c/9124/1//COMMIT_MSG@10 PS1, Line 10: HBase table names. It is relatively straitforward to make nit: straightforward http://gerrit.cloudera.org:8080/#/c/9124/1//COMMIT_MSG@14 PS1, Line 14: Testing: With an Impala built with a local override to use The documentation in docs/topics/impala_hbase.xml refers "hbase.table.name" and also needs to be fixed. (Or maybe Hive shouldn't have changed this.) Is a simpler reproduction to create a table from Hive and then just query it here? http://gerrit.cloudera.org:8080/#/c/9124/1/fe/src/main/java/org/apache/impala/catalog/HBaseTable.java File fe/src/main/java/org/apache/impala/catalog/HBaseTable.java: http://gerrit.cloudera.org:8080/#/c/9124/1/fe/src/main/java/org/apache/impala/catalog/HBaseTable.java@450 PS1, Line 450: // the original table name, and if that fails, try looking for the As Dimitris points out, this would be clearer if we said: We look up the HBase Table name in the following order: * Try looking up HBASE_TABLE_NAME_KEY in SERDEPROPERTIES; return it in found. * Try looking up HBASE_TABLE_NAME_COMPAT_KEY in SERDEPROPERTIES; return it if found. * If the table is in the default DB, return the table name. * Return db.tablename. My point is that I'm suggesting we rename HBASE_TABLE_NAME to HBASE_TABLE_NAME_KEY, since it's the key in SERDEPROPERTIES. By my look, the only users are here in this file, so it's not a hard change. (If it was pervasive, I wouldn't be bothered if we kept it the way it is.) -- To view, visit http://gerrit.cloudera.org:8080/9124 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Id7c5b62f38be275762771ecf39cf898f06f8138b Gerrit-Change-Number: 9124 Gerrit-PatchSet: 1 Gerrit-Owner: Zach Amsden <zams...@cloudera.com> Gerrit-Reviewer: Dimitris Tsirogiannis <dtsirogian...@cloudera.com> Gerrit-Reviewer: Philip Zeyliger <phi...@cloudera.com> Gerrit-Reviewer: Zach Amsden <zams...@cloudera.com> Gerrit-Comment-Date: Wed, 24 Jan 2018 23:37:19 +0000 Gerrit-HasComments: Yes
