Hao Hao has posted comments on this change. ( http://gerrit.cloudera.org:8080/13375 )
Change subject: IMPALA-8504 (part 2): Support CREATE TABLE statement with Kudu/HMS integration ...................................................................... Patch Set 4: (9 comments) http://gerrit.cloudera.org:8080/#/c/13375/4/fe/src/main/java/org/apache/impala/catalog/KuduTable.java File fe/src/main/java/org/apache/impala/catalog/KuduTable.java: http://gerrit.cloudera.org:8080/#/c/13375/4/fe/src/main/java/org/apache/impala/catalog/KuduTable.java@172 PS4, Line 172: org.apache.hadoop.hive.metastore.api.Table msTbl, : boolean isHMSIntegrationEnabled > these can fit on the same line It exceeds the line length limit (91). http://gerrit.cloudera.org:8080/#/c/13375/4/fe/src/main/java/org/apache/impala/catalog/KuduTable.java@231 PS4, Line 231: if (hmsHosts != null && kuduHmsHosts != null &&hmsHosts.equals(kuduHmsHosts)) { > nit: separate by a space Done http://gerrit.cloudera.org:8080/#/c/13375/4/fe/src/main/java/org/apache/impala/catalog/KuduTable.java@234 PS4, Line 234: throw new ImpalaRuntimeException( : String.format("Kudu is integrated with a different Hive Metastore " + : "than that used by Impala, Kudu is configured to use the HMS: " + : "%s, while Impala is configured to use the HMS: %s", : kuduHmsUris, hmsUris)); > readability nit: remove 'else' and move this out of the scope if/else scope Done http://gerrit.cloudera.org:8080/#/c/13375/4/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java File fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java: http://gerrit.cloudera.org:8080/#/c/13375/4/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@1930 PS4, Line 1930: !KuduTable.isHMSIntegrationEnabledAndValidate(masterHosts, : hmsUris > formatting - put everything after the '=' on the next line, indented by 4 Done http://gerrit.cloudera.org:8080/#/c/13375/4/fe/src/main/java/org/apache/impala/util/MetaStoreUtil.java File fe/src/main/java/org/apache/impala/util/MetaStoreUtil.java: http://gerrit.cloudera.org:8080/#/c/13375/4/fe/src/main/java/org/apache/impala/util/MetaStoreUtil.java@87 PS4, Line 87: public static final String HIVE_METASTORE_URIS_KEY = : "hive.metastore.uris"; > This can fit on 1 line Done http://gerrit.cloudera.org:8080/#/c/13375/4/fe/src/main/java/org/apache/impala/util/MetaStoreUtil.java@120 PS4, Line 120: /** : * Return the value of thrift URI for the remote Hive Metastore. : */ : public static String getHiveMetastoreUrisKeyValue(IMetaStoreClient client) : throws ConfigValSecurityException, TException { : return client.getConfigValue( : HIVE_METASTORE_URIS_KEY, DEFAULT_HIVE_METASTORE_URIS); : } > Why to create this shortcut instead of just using getMetastoreConfigValue() This is to ensure always use the correct key (HIVE_METASTORE_URIS_KEY) when calling this function. http://gerrit.cloudera.org:8080/#/c/13375/4/tests/custom_cluster/test_kudu.py File tests/custom_cluster/test_kudu.py: http://gerrit.cloudera.org:8080/#/c/13375/4/tests/custom_cluster/test_kudu.py@129 PS4, Line 129: manged > typo Done http://gerrit.cloudera.org:8080/#/c/13375/4/tests/query_test/test_kudu.py File tests/query_test/test_kudu.py: http://gerrit.cloudera.org:8080/#/c/13375/4/tests/query_test/test_kudu.py@93 PS4, Line 93: @SkipIfKudu.hms_integration_enabled > Why would a test like this, which is focused on scanning kudu tables, need Done http://gerrit.cloudera.org:8080/#/c/13375/4/tests/query_test/test_kudu.py@175 PS4, Line 175: @SkipIfKudu.hms_integration_enabled > I assume that for cases like these, where the table is modified outside of I am not sure it is the case, as far as my understanding of IMPALA-4828 is due to the table schema is cached in impala which is different from the one in Kudu. While with HMS integration, Kudu only automatically update the schema in the HMS. Therefore, I don't see the behavior will change. Or I am missing something? -- To view, visit http://gerrit.cloudera.org:8080/13375 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Icffe412395f47f5e07d97bad457020770cfa7502 Gerrit-Change-Number: 13375 Gerrit-PatchSet: 4 Gerrit-Owner: Hao Hao <hao....@cloudera.com> Gerrit-Reviewer: Alexey Serbin <aser...@cloudera.com> Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com> Gerrit-Reviewer: Grant Henke <granthe...@apache.org> Gerrit-Reviewer: Hao Hao <hao....@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Thomas Marshall <tmarsh...@cloudera.com> Gerrit-Comment-Date: Sun, 02 Jun 2019 20:57:00 +0000 Gerrit-HasComments: Yes
