Csaba Ringhofer has posted comments on this change. ( http://gerrit.cloudera.org:8080/19378 )
Change subject: IMPALA:11808: Added support for reload event in catalogD ...................................................................... Patch Set 6: (7 comments) http://gerrit.cloudera.org:8080/#/c/19378/6/be/src/catalog/catalog-server.cc File be/src/catalog/catalog-server.cc: http://gerrit.cloudera.org:8080/#/c/19378/6/be/src/catalog/catalog-server.cc@127 PS6, Line 127: This configuration is used " : "to fire a refresh/invalidate table event to the HMS and other event processors " : "can process this event. The default value is false, so impala will not fire this " : "event. If enabled, impala will fire this event and other catalogD will process it It is not clear to me from the description that this config affects both firing and processing of the event http://gerrit.cloudera.org:8080/#/c/19378/6/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/19378/6/fe/src/compat-apache-hive-3/java/org/apache/impala/compat/MetastoreShim.java@442 PS6, Line 442: return Collections.EMPTY_LIST; I would prefer throwing UnsupportedOperationException http://gerrit.cloudera.org:8080/#/c/19378/6/fe/src/compat-apache-hive-3/java/org/apache/impala/compat/MetastoreShim.java@447 PS6, Line 447: MetastoreNotificationException UnsupportedOperationException seem more fitting here. http://gerrit.cloudera.org:8080/#/c/19378/6/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/19378/6/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java@2474 PS6, Line 2474: enableProcessingReloadEvent Isn't it called enableReloadEvents()? http://gerrit.cloudera.org:8080/#/c/19378/6/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java@2476 PS6, Line 2476: will will not be processed? http://gerrit.cloudera.org:8080/#/c/19378/6/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java@2531 PS6, Line 2531: //we always treat the table as non-transactional so all the files are reloaded nit: missing space http://gerrit.cloudera.org:8080/#/c/19378/6/tests/custom_cluster/test_events_custom_configs.py File tests/custom_cluster/test_events_custom_configs.py: http://gerrit.cloudera.org:8080/#/c/19378/6/tests/custom_cluster/test_events_custom_configs.py@262 PS6, Line 262: check_self_events("refresh {}.{} partition(year=2022)" nit: +1 black line after the nested function may make it easier to read -- To view, visit http://gerrit.cloudera.org:8080/19378 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ic62d58837d356dc2113f3c0904228ac9de484136 Gerrit-Change-Number: 19378 Gerrit-PatchSet: 6 Gerrit-Owner: Sai Hemanth Gantasala <saihema...@cloudera.com> Gerrit-Reviewer: Abhishek Rawat <ara...@cloudera.com> Gerrit-Reviewer: Csaba Ringhofer <csringho...@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: Wed, 04 Jan 2023 21:19:54 +0000 Gerrit-HasComments: Yes
