Sai Hemanth Gantasala has posted comments on this change. ( http://gerrit.cloudera.org:8080/21031 )
Change subject: IMPALA-12709: Add support for hierarchical metastore event processing ...................................................................... Patch Set 19: (12 comments) http://gerrit.cloudera.org:8080/#/c/21031/19//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/21031/19//COMMIT_MSG@28 PS19, Line 28: relating nit: related http://gerrit.cloudera.org:8080/#/c/21031/19/be/src/catalog/catalog-server.cc File be/src/catalog/catalog-server.cc: http://gerrit.cloudera.org:8080/#/c/21031/19/be/src/catalog/catalog-server.cc@235 PS19, Line 235: db_event_executors IMO, db_event_executors and table_event_executors could be a query option flag rather than start-up flag. http://gerrit.cloudera.org:8080/#/c/21031/19/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/21031/19/fe/src/compat-apache-hive-3/java/org/apache/impala/compat/MetastoreShim.java@658 PS19, Line 658: PseudoCommitTxnEvent Why did you feel the need to introduce a new pseudo class? http://gerrit.cloudera.org:8080/#/c/21031/19/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/21031/19/fe/src/compat-hive-3/java/org/apache/impala/compat/MetastoreShim.java@846 PS19, Line 846: tableWriteIds_ = catalog_.removeWriteIds(txnId_); I think we'll be removing writeIds for the transaction if the event is self event. Is this desired? http://gerrit.cloudera.org:8080/#/c/21031/19/fe/src/compat-hive-3/java/org/apache/impala/compat/MetastoreShim.java@966 PS19, Line 966: catalogOpExecutor What happens when different table event executors try to modify different tables in same database? Do we observe any timeouts or slowness? http://gerrit.cloudera.org:8080/#/c/21031/19/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/21031/19/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@4081 PS19, Line 4081: LOG.debug("Add {} write ids {} to table {}.{} for event {}", status, writeIds, dbName, nit: adding? http://gerrit.cloudera.org:8080/#/c/21031/19/fe/src/main/java/org/apache/impala/catalog/events/DBBarrierEvent.java File fe/src/main/java/org/apache/impala/catalog/events/DBBarrierEvent.java: http://gerrit.cloudera.org:8080/#/c/21031/19/fe/src/main/java/org/apache/impala/catalog/events/DBBarrierEvent.java@24 PS19, Line 24: DBBarrierEvent nit: Need some explanation regarding what is the objective of this new class. http://gerrit.cloudera.org:8080/#/c/21031/19/fe/src/main/java/org/apache/impala/catalog/events/DBBarrierEvent.java@55 PS19, Line 55: public void decrExpectedProceedCount() { expectedProceedCount_.decrementAndGet(); } nit: should we log the counter of 'expectedProceedCount_' whenever we incr or decr the counter for debugging purposes? http://gerrit.cloudera.org:8080/#/c/21031/19/fe/src/main/java/org/apache/impala/catalog/events/ExternalEventsProcessor.java File fe/src/main/java/org/apache/impala/catalog/events/ExternalEventsProcessor.java: http://gerrit.cloudera.org:8080/#/c/21031/19/fe/src/main/java/org/apache/impala/catalog/events/ExternalEventsProcessor.java@52 PS19, Line 52: void hold(); Why is it any different from pause state? http://gerrit.cloudera.org:8080/#/c/21031/19/fe/src/main/java/org/apache/impala/service/BackendConfig.java File fe/src/main/java/org/apache/impala/service/BackendConfig.java: http://gerrit.cloudera.org:8080/#/c/21031/19/fe/src/main/java/org/apache/impala/service/BackendConfig.java@535 PS19, Line 535: remove_processor_threshold nit: should we rename this 'event_executor_idle_timeout'? because "getRemoveProcessorThreshold()" seems out of place, I think "getEventExecutorIdleTimeout" sounds good. http://gerrit.cloudera.org:8080/#/c/21031/19/fe/src/main/java/org/apache/impala/service/JniCatalog.java File fe/src/main/java/org/apache/impala/service/JniCatalog.java: http://gerrit.cloudera.org:8080/#/c/21031/19/fe/src/main/java/org/apache/impala/service/JniCatalog.java@222 PS19, Line 222: BackendConfig.INSTANCE.getHMSPollingIntervalInSeconds() * 1000; Even with this feature disabled, do we want to deal with polling interval in milliseconds? http://gerrit.cloudera.org:8080/#/c/21031/19/tests/custom_cluster/test_events_custom_configs.py File tests/custom_cluster/test_events_custom_configs.py: http://gerrit.cloudera.org:8080/#/c/21031/19/tests/custom_cluster/test_events_custom_configs.py@1301 PS19, Line 1301: self._run_self_events_test(unique_database, vector.get_value('exec_option'), refactoring tip: We can introduce only one new test and call "_run_self_events_test" with and without impala enabled. -- To view, visit http://gerrit.cloudera.org:8080/21031 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I76d8a739f9db6d40f01028bfd786a85d83f9e5d6 Gerrit-Change-Number: 21031 Gerrit-PatchSet: 19 Gerrit-Owner: Anonymous Coward <k.venureddy2...@gmail.com> Gerrit-Reviewer: Anonymous Coward <cclive1...@gmail.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: Thu, 16 May 2024 05:37:18 +0000 Gerrit-HasComments: Yes
