Riza Suminto 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 44: (19 comments) Thank you for keeping up with my reviews. I think this is very close now. I only have few minor comments remaining. 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> : > Done 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); > Done. Have rephrased the java doc. Done 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: > Done Done http://gerrit.cloudera.org:8080/#/c/21031/42/fe/src/main/java/org/apache/impala/catalog/events/DbEventExecutor.java@468 PS42, Line 468: */ > Done 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 > Done 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() { > If force is true, dbProcessor.clear() is called and it clears inEvents_, ba Ack 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 > Done Done http://gerrit.cloudera.org:8080/#/c/21031/44/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/44/fe/src/main/java/org/apache/impala/catalog/events/DbEventExecutor.java@171 PS44, Line 171: private final Object usableLock_ = new Object(); : private boolean usable_ = true; nit: I think processorLock_ and isTerminating_ term are more descriptive than usableLock_ and usable_. http://gerrit.cloudera.org:8080/#/c/21031/44/fe/src/main/java/org/apache/impala/catalog/events/DbEventExecutor.java@340 PS44, Line 340: private boolean isUsable() { : synchronized (usableLock_) { : return usable_; : } : } Taking example from IMPALA-13884, consider writing the stacktrace into debug log into like this: private boolean isTerminating() { synchronized (processorLock_) { if (isTerminating && LOG.isDebugEnabled()) { // Get the caller stacktrace and convert it into string. StackTraceElement[] stack = Thread.currentThread().getStackTrace(); // Skip the first method which is always java.lang.Thread.getStackTrace(). String st = Arrays.stream(stack).skip(1) .map(StackTraceElement::toString) .collect(Collectors.joining("\n\tat ")); LOG.debug("Processor is not usable for db: {}. Caller stacktrace: {}", dbName_, st); } return isTerminating; } } Then, other callsites can be simplified without printing LOG.debug like this: if (isTerminating()) return; http://gerrit.cloudera.org:8080/#/c/21031/44/fe/src/main/java/org/apache/impala/catalog/events/DbEventExecutor.java@396 PS44, Line 396: if (!isUsable()) { : LOG.debug("Processor is not usable for db: {}", dbName_); : return; : } This pattern is repeated in multiple places. I understand this serve as debug point. Please consider moving the debug log inside the isUsable() function. See this suggestion: https://gerrit.cloudera.org/c/21031/42..44#344 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 > Done 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 > Done 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 > Done 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 > Done. Have rephrased the java doc. 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() > Done 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)} > Done Done http://gerrit.cloudera.org:8080/#/c/21031/42/fe/src/main/java/org/apache/impala/catalog/events/TableEventExecutor.java@364 PS42, Line 364: * > Done Done http://gerrit.cloudera.org:8080/#/c/21031/44/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/44/fe/src/main/java/org/apache/impala/catalog/events/TableEventExecutor.java@198 PS44, Line 198: private boolean isUsable() { : synchronized (usableLock_) { : return usable_; : } : } Same suggestion as https://gerrit.cloudera.org/c/21031/42..44#344 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 > Done 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: 44 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: Wed, 16 Apr 2025 03:11:46 +0000 Gerrit-HasComments: Yes
