Quanlong Huang has posted comments on this change. ( http://gerrit.cloudera.org:8080/19155 )
Change subject: IMPALA-11626: Handled COMMIT_COMPACTION_EVENT from HMS ...................................................................... Patch Set 4: (15 comments) http://gerrit.cloudera.org:8080/#/c/19155/4//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/19155/4//COMMIT_MSG@9 PS4, Line 9: HMS emits an event when a compaction is committed For a partitioned table, does HMS still emit a single event if all partitions got compacted? http://gerrit.cloudera.org:8080/#/c/19155/4/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java File fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java: http://gerrit.cloudera.org:8080/#/c/19155/4/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@2292 PS4, Line 2292: ,DatabaseNotFoundException nit: DatabaseNotFoundException is a CatalogException so we can remove it. http://gerrit.cloudera.org:8080/#/c/19155/4/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@2299 PS4, Line 2299: tbl == null || nit: remove null check since it is covered by subsequent check of 'instanceof' http://gerrit.cloudera.org:8080/#/c/19155/4/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@2304 PS4, Line 2304: nit: our code style uses 4 spaces for continuation ident. http://gerrit.cloudera.org:8080/#/c/19155/4/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@2308 PS4, Line 2308: nit: 4 spaces http://gerrit.cloudera.org:8080/#/c/19155/4/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@2310 PS4, Line 2310: if( nit: need a space after "if" http://gerrit.cloudera.org:8080/#/c/19155/4/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java File fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java: http://gerrit.cloudera.org:8080/#/c/19155/4/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java@220 PS4, Line 220: return new CommitCompactionEvent(catalogOpExecutor_, metrics, event); This can also be considered as transactional events that trigger incremental refresh. It'd be better to move this to line 196 since transactional events are handled there. http://gerrit.cloudera.org:8080/#/c/19155/4/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java@2492 PS4, Line 2492: private final String dbName_; : : private final String tblName_; : These two are already defined in the base class. We can remove them here. http://gerrit.cloudera.org:8080/#/c/19155/4/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java@2502 PS4, Line 2502: nit: 4 spaces for continuation ident http://gerrit.cloudera.org:8080/#/c/19155/4/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java@2505 PS4, Line 2505: getCommitCompactionMessage If this method only exists in CDP Hive, we need to add a wrapper for it in MetastoreShim. So Impala can still compile with Apache Hive3. Otherwise, this patch will encounter build failures in pre-commit jobs. We have MetastoreShim for both Hive versions: CDP Hive: https://github.com/apache/impala/blob/390a932064ba48c44e70e431f2c20e53beddd970/fe/src/compat-hive-3/java/org/apache/impala/compat/MetastoreShim.java Apache Hive3: https://github.com/apache/impala/blob/390a932064ba48c44e70e431f2c20e53beddd970/fe/src/compat-apache-hive-3/java/org/apache/impala/compat/MetastoreShim.java http://gerrit.cloudera.org:8080/#/c/19155/4/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java@2506 PS4, Line 2506: tblName_ = commitCompactionMessage.getTableName(); : dbName_ = commitCompactionMessage.getDbname(); This is related to the above comment about removing these fields. Are these the same as we get from 'event' in the base class? https://github.com/apache/impala/blob/390a932064ba48c44e70e431f2c20e53beddd970/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java#L466-L467 http://gerrit.cloudera.org:8080/#/c/19155/4/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java@2515 PS4, Line 2515: nit: 4 spaces http://gerrit.cloudera.org:8080/#/c/19155/4/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java@2518 PS4, Line 2518: nit: please fix the idention to align with other code, e.g. https://github.com/apache/impala/blob/390a932064ba48c44e70e431f2c20e53beddd970/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java#L1298-L1302 http://gerrit.cloudera.org:8080/#/c/19155/4/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java@2520 PS4, Line 2520: , nit: need a space after comma http://gerrit.cloudera.org:8080/#/c/19155/4/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java@2536 PS4, Line 2536: Not skipping the event since there can be multiple tables involved This comment confused me. Isn't this a single table event? Maybe I misunderstood something. It'd be nice if we can update this comment. BTW, please fix the code style that each line of the method comment should starts with a star '*', and the first line should be '/**'. -- To view, visit http://gerrit.cloudera.org:8080/19155 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I464faedb4e3bbcd417bab2e3cb0d57e339d42605 Gerrit-Change-Number: 19155 Gerrit-PatchSet: 4 Gerrit-Owner: Sai Hemanth Gantasala <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Quanlong Huang <[email protected]> Gerrit-Reviewer: Sai Hemanth Gantasala <[email protected]> Gerrit-Comment-Date: Tue, 20 Dec 2022 09:08:24 +0000 Gerrit-HasComments: Yes
