k.venureddy2...@gmail.com 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 43: (16 comments) http://gerrit.cloudera.org:8080/#/c/21031/42/docs/topics/impala_metadata.xml File docs/topics/impala_metadata.xml: http://gerrit.cloudera.org:8080/#/c/21031/42/docs/topics/impala_metadata.xml@307 PS42, Line 307: : </p> : > Consider dropping this sentence. Done http://gerrit.cloudera.org:8080/#/c/21031/42/fe/src/main/java/org/apache/impala/catalog/events/DbEventExecutor.java File fe/src/main/java/org/apache/impala/catalog/events/DbEventExecutor.java: http://gerrit.cloudera.org:8080/#/c/21031/42/fe/src/main/java/org/apache/impala/catalog/events/DbEventExecutor.java@188 PS42, Line 188: inEvents_.offer(event); > This lock also protect inEvents_.poll(), barrierEvents_.poll(), and dbEvent Done. Have rephrased the java doc. http://gerrit.cloudera.org:8080/#/c/21031/42/fe/src/main/java/org/apache/impala/catalog/events/DbEventExecutor.java@459 PS42, Line 459: * This method is invoked in the following cases: > Please document when this method must be called. Done http://gerrit.cloudera.org:8080/#/c/21031/42/fe/src/main/java/org/apache/impala/catalog/events/DbEventExecutor.java@468 PS42, Line 468: */ > Remove this method and use the overload with 'delta' parameter instead. Done http://gerrit.cloudera.org:8080/#/c/21031/42/fe/src/main/java/org/apache/impala/catalog/events/DbEventExecutor.java@477 PS42, Line 477: @return Outstanding event count > Add error message. Done http://gerrit.cloudera.org:8080/#/c/21031/42/fe/src/main/java/org/apache/impala/catalog/events/DbEventExecutor.java@529 PS42, Line 529: e void clearInternal() { > Should this always happen if 'force' is True? If force is true, dbProcessor.clear() is called and it clears inEvents_, barrierEvents_ and tableProcessors_ and sets lastEventQueuedTime_ to 0 as well. So dbProcessor.canBeRemoved() returns true. http://gerrit.cloudera.org:8080/#/c/21031/42/fe/src/main/java/org/apache/impala/catalog/events/DbEventExecutor.java@548 PS42, Line 548: MetastoreEventsProcessor.shutdownAndAwaitTermination(service > Add error message. Done http://gerrit.cloudera.org:8080/#/c/21031/42/fe/src/main/java/org/apache/impala/catalog/events/EventExecutorService.java File fe/src/main/java/org/apache/impala/catalog/events/EventExecutorService.java: http://gerrit.cloudera.org:8080/#/c/21031/42/fe/src/main/java/org/apache/impala/catalog/events/EventExecutorService.java@72 PS42, Line 72: /** : * Database name to DbEventExecutor map. This map is mutated only by : * DbEventExecutor through > Please document that This map should be mutated only by DbeventExecutor thr Done http://gerrit.cloudera.org:8080/#/c/21031/42/fe/src/main/java/org/apache/impala/catalog/events/EventExecutorService.java@133 PS42, Line 133: * {@link Exte > I think dispatch(), shutdown(), and clear() should all be a synchronized me Done http://gerrit.cloudera.org:8080/#/c/21031/42/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEventsProcessor.java File fe/src/main/java/org/apache/impala/catalog/events/MetastoreEventsProcessor.java: http://gerrit.cloudera.org:8080/#/c/21031/42/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEventsProcessor.java@1231 PS42, Line 1231: an invalidat > nit: an invalidate Done http://gerrit.cloudera.org:8080/#/c/21031/42/fe/src/main/java/org/apache/impala/catalog/events/TableEventExecutor.java File fe/src/main/java/org/apache/impala/catalog/events/TableEventExecutor.java: http://gerrit.cloudera.org:8080/#/c/21031/42/fe/src/main/java/org/apache/impala/catalog/events/TableEventExecutor.java@125 PS42, Line 125: * {@link TableProcessor#process()}, since lock > This lock also protect events_.poll() and tableEventExecutor_.decrOutstandi Done. Have rephrased the java doc. http://gerrit.cloudera.org:8080/#/c/21031/42/fe/src/main/java/org/apache/impala/catalog/events/TableEventExecutor.java@328 PS42, Line 328: } > Please document when this method must be called. Done http://gerrit.cloudera.org:8080/#/c/21031/42/fe/src/main/java/org/apache/impala/catalog/events/TableEventExecutor.java@337 PS42, Line 337: outstandingEventCount_.incrementAndGet() > Remove this method and use the overload with 'delta' parameter instead. Done http://gerrit.cloudera.org:8080/#/c/21031/42/fe/src/main/java/org/apache/impala/catalog/events/TableEventExecutor.java@346 PS42, Line 346: {@link TableEventExecutor#deleteTableProcessor(String)} > Add error message. Done http://gerrit.cloudera.org:8080/#/c/21031/42/fe/src/main/java/org/apache/impala/catalog/events/TableEventExecutor.java@364 PS42, Line 364: * > Add error message. Done http://gerrit.cloudera.org:8080/#/c/21031/42/tests/custom_cluster/test_event_processing_perf.py File tests/custom_cluster/test_event_processing_perf.py: http://gerrit.cloudera.org:8080/#/c/21031/42/tests/custom_cluster/test_event_processing_perf.py@43 PS42, Line 43: db_count = 3 : table_count = 3 : partition_count = 100 : insert_nonpartition_values_count = 100 : insert_nonpartition_repeat_count = 2 > Consider lowering this test dimension so that we can regularly run this tes Done -- 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: 43 Gerrit-Owner: Anonymous Coward <k.venureddy2...@gmail.com> Gerrit-Reviewer: Anonymous Coward <cclive1...@gmail.com> Gerrit-Reviewer: Anonymous Coward <k.venureddy2...@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: Riza Suminto <riza.sumi...@cloudera.com> Gerrit-Reviewer: Sai Hemanth Gantasala <saihema...@cloudera.com> Gerrit-Comment-Date: Tue, 15 Apr 2025 14:33:46 +0000 Gerrit-HasComments: Yes
