Sai Hemanth Gantasala has posted comments on this change. ( http://gerrit.cloudera.org:8080/19155 )
Change subject: IMPALA-11626: Handle COMMIT_COMPACTION_EVENT from HMS ...................................................................... Patch Set 8: (6 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 > I see. Thanks for the explanation! Ack http://gerrit.cloudera.org:8080/#/c/19155/7/fe/src/compat-apache-hive-3/java/org/apache/impala/compat/MetastoreShim.java File fe/src/compat-apache-hive-3/java/org/apache/impala/compat/MetastoreShim.java: http://gerrit.cloudera.org:8080/#/c/19155/7/fe/src/compat-apache-hive-3/java/org/apache/impala/compat/MetastoreShim.java@441 PS7, Line 441: public static Map<String, Object> getFieldsFromReloadEvent(NotificationEvent event) > Why do we need to change the return type of this? I intend to change the other method return type. My bad!! http://gerrit.cloudera.org:8080/#/c/19155/7/fe/src/compat-apache-hive-3/java/org/apache/impala/compat/MetastoreShim.java@449 PS7, Line 449: > nit: our code style tend to put the type and var name at the same line. Ack http://gerrit.cloudera.org:8080/#/c/19155/7/fe/src/compat-hive-3/java/org/apache/impala/compat/MetastoreShim.java File fe/src/compat-hive-3/java/org/apache/impala/compat/MetastoreShim.java: http://gerrit.cloudera.org:8080/#/c/19155/7/fe/src/compat-hive-3/java/org/apache/impala/compat/MetastoreShim.java@570 PS7, Line 570: This method extracts the partition name field from the : * notification event and returns it in the form > Please update this Ack http://gerrit.cloudera.org:8080/#/c/19155/7/fe/src/compat-hive-3/java/org/apache/impala/compat/MetastoreShim.java@577 PS7, Line 577: throws MetastoreNotificationException > nit: it seems no exceptions will be thrown It is possible that the new impala client can talk to the old HMS where there is no implementation of getCommitCompactionMessage() or it is possible that the event can message is incorrectly encoded and we can hit an error during decoding. In these cases, it is nice to catch this exception in the form of MetastoreNotificationException. What do you think? http://gerrit.cloudera.org:8080/#/c/19155/7/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/7/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java@2632 PS7, Line 2632: MetastoreEvent > I think we should extend MetastoreTableEvent so we can reuse its reload met There is no method in CatalogServiceCatalog that would only refresh the file metadata of the table. reloadTableIfExists() method would reload table metadata and file metadata which would be an overhead here. CommitCompaction event would only change the file metadata. So I thought I'll implement a method that does only file metadata reload. Are you ok with this? -- 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: 8 Gerrit-Owner: Sai Hemanth Gantasala <saihema...@cloudera.com> Gerrit-Reviewer: Daniel Becker <daniel.bec...@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Quanlong Huang <huangquanl...@gmail.com> Gerrit-Reviewer: Sai Hemanth Gantasala <saihema...@cloudera.com> Gerrit-Comment-Date: Tue, 31 Jan 2023 00:09:21 +0000 Gerrit-HasComments: Yes
