Sai Hemanth Gantasala has posted comments on this change. ( http://gerrit.cloudera.org:8080/19155 )
Change subject: IMPALA-11626: Handled COMMIT_COMPACTION_EVENT from HMS ...................................................................... Patch Set 5: (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 partitio It'll be a single commit compaction event always. Consider a partitioned table foo, > Alter table foo compact 'minor'; --> will fire single commit compaction event > at the table level, partition name would be null, in this case, we need to > reload whole table file metadata. >Alter table foo partition(i=1) compact 'minor'; --> will fire single commit >compaction event for a single partition i=1 of the table foo. 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: { > nit: DatabaseNotFoundException is a CatalogException so we can remove it. Ack http://gerrit.cloudera.org:8080/#/c/19155/4/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@2299 PS4, Line 2299: !(tbl instanceo > nit: remove null check since it is covered by subsequent check of 'instance Ack http://gerrit.cloudera.org:8080/#/c/19155/4/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@2304 PS4, Line 2304: Coll > nit: our code style uses 4 spaces for continuation ident. Ack http://gerrit.cloudera.org:8080/#/c/19155/4/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@2308 PS4, Line 2308: Coll > nit: 4 spaces Ack 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" Ack 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: case INSERT: > This can also be considered as transactional events that trigger incrementa This event can come as a standalone event after a major/minor compaction apart from an incremental refresh. I thought this would be the right place. Also, since the incremental refresh event processing is configurable I thought it is right to put it here. @yu-wen also thinks this is the right place. What do you think? http://gerrit.cloudera.org:8080/#/c/19155/4/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java@2492 PS4, Line 2492: processTableReload(); : } : } else { : > These two are already defined in the base class. We can remove them here. Ack http://gerrit.cloudera.org:8080/#/c/19155/4/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java@2502 PS4, Line 2502: ivate vo > nit: 4 spaces for continuation ident Ack http://gerrit.cloudera.org:8080/#/c/19155/4/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java@2505 PS4, Line 2505: > If this method only exists in CDP Hive, we need to add a wrapper for it in Ack http://gerrit.cloudera.org:8080/#/c/19155/4/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java@2506 PS4, Line 2506: // Ignore event if table or database is not in catalog. Throw exception if : // refresh fails. If the partition does not > This is related to the above comment about removing these fields. Are these Ack http://gerrit.cloudera.org:8080/#/c/19155/4/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java@2515 PS4, Line 2515: + "p > nit: 4 spaces Ack http://gerrit.cloudera.org:8080/#/c/19155/4/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java@2518 PS4, Line 2518: Joiner.on(', > nit: please fix the idention to align with other code, e.g. Ack 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 Ack http://gerrit.cloudera.org:8080/#/c/19155/4/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java@2536 PS4, Line 2536: getFullyQualifiedTblName()), e); > This comment confused me. Isn't this a single table event? Maybe I misunder Ack -- 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: 5 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: Wed, 18 Jan 2023 17:42:32 +0000 Gerrit-HasComments: Yes
