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 33: (15 comments) http://gerrit.cloudera.org:8080/#/c/21031/33//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/21031/33//COMMIT_MSG@20 PS33, Line 20: Existing metastore event processing is turned into multi-level : event processing with enable_hierarchical_event_processing flag. Please add short paragraph describing the relationship between DBEventExecutor, TableEventExecutor. Do they have fixed number all the time? I noticed this as been asked before, hence my request to write it down here in commit message. Please describe in the paragraph how linearizability is guaranteed. Is it guaranteed because event from same DB is guaranteed to go to the same DBEventExecutor, and event from same Table is guaranteed to go to the same TableEventExecutor? http://gerrit.cloudera.org:8080/#/c/21031/33/be/src/catalog/catalog-server.cc File be/src/catalog/catalog-server.cc: http://gerrit.cloudera.org:8080/#/c/21031/33/be/src/catalog/catalog-server.cc@222 PS33, Line 222: db_event_executors nit: num_db_event_executors sounds better. http://gerrit.cloudera.org:8080/#/c/21031/33/be/src/catalog/catalog-server.cc@230 PS33, Line 230: table_event_executors nit: num_table_event_executors_per_db sounds better. http://gerrit.cloudera.org:8080/#/c/21031/33/be/src/catalog/catalog-server.cc@237 PS33, Line 237: remove_processor_threshold nit: Since ses_ is scheduleAtFixedRate, can you rename this flag to reflect time period? Maybe DEFINE_int32(max_event_processor_idle_ms, 60000, http://gerrit.cloudera.org:8080/#/c/21031/33/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/33/fe/src/main/java/org/apache/impala/catalog/events/DBEventExecutor.java@45 PS33, Line 45: DB event executor nit: Please consistently use DBEventExecutor in documentation. It helps reader to search and highlight. Same comment for "table event executor", "DB processors", and "table processor". http://gerrit.cloudera.org:8080/#/c/21031/33/fe/src/main/java/org/apache/impala/catalog/events/DBEventExecutor.java@53 PS33, Line 53: a list of Is the number of TableEventExecutor per DBEventExecutor is fixed all the time? http://gerrit.cloudera.org:8080/#/c/21031/33/fe/src/main/java/org/apache/impala/catalog/events/DBEventExecutor.java@71 PS33, Line 71: // Executor name Move the comment above the field declaration. What is an example/pattern of executorName_? name_ is probably more concise. http://gerrit.cloudera.org:8080/#/c/21031/33/fe/src/main/java/org/apache/impala/catalog/events/DBEventExecutor.java@73 PS33, Line 73: ses_ Please use more descriptive variable name. Maybe service_? http://gerrit.cloudera.org:8080/#/c/21031/33/fe/src/main/java/org/apache/impala/catalog/events/DBEventExecutor.java@91 PS33, Line 91: usableLock_ usableLock_ seems to protect more than just usable_. Please document what other routine need to obtain usableLock_, when this executor is not usable, and what the expected behavior when this executor is not usable. http://gerrit.cloudera.org:8080/#/c/21031/33/fe/src/main/java/org/apache/impala/catalog/events/DBEventExecutor.java@125 PS33, Line 125: private final Object usableLock_ = new Object(); : private boolean usable_ = true; usableLock_ seems to protect more than just usable_. Please document what other routine need to obtain usableLock_, when this executor is not usable, and what the expected behavior when this executor is not usable. http://gerrit.cloudera.org:8080/#/c/21031/33/fe/src/main/java/org/apache/impala/catalog/events/DBEventExecutor.java@231 PS33, Line 231: synchronized (usableLock_) { : usable_ = false; : } setUsable(false); http://gerrit.cloudera.org:8080/#/c/21031/33/fe/src/main/java/org/apache/impala/catalog/events/DBEventExecutor.java@439 PS33, Line 439: : private TableEventExecutor getTableEventExecutor(String fqTableName) { Please write short documentation for this method and others that does not have one. >From dispatchTableEvent(), why tableToEventExecutor_.get() is called again >later in assignTableEventExecutor()? Maybe it is better to merge this getTableEventExecutor and assignTableEventExecutor (the only caller). Let assignTableEventExecutor ssignTableEventExecutor figure out which TableEventExecutor to pick for given fqTableName. http://gerrit.cloudera.org:8080/#/c/21031/33/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/33/fe/src/main/java/org/apache/impala/catalog/events/TableEventExecutor.java@53 PS33, Line 53: Table name nit: Fully qualified table name http://gerrit.cloudera.org:8080/#/c/21031/33/fe/src/main/java/org/apache/impala/catalog/events/TableEventExecutor.java@55 PS33, Line 55: // Executor name Move the comment above the field declaration. What is an example/pattern of executorName_? name_ is probably more concise. http://gerrit.cloudera.org:8080/#/c/21031/33/fe/src/main/java/org/apache/impala/catalog/events/TableEventExecutor.java@57 PS33, Line 57: ses_ Please use more descriptive variable name. Maybe service_? -- 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: 33 Gerrit-Owner: Anonymous Coward <[email protected]> Gerrit-Reviewer: Anonymous Coward <[email protected]> Gerrit-Reviewer: Anonymous Coward <[email protected]> Gerrit-Reviewer: Csaba Ringhofer <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Quanlong Huang <[email protected]> Gerrit-Reviewer: Riza Suminto <[email protected]> Gerrit-Reviewer: Sai Hemanth Gantasala <[email protected]> Gerrit-Comment-Date: Sun, 05 Jan 2025 06:39:15 +0000 Gerrit-HasComments: Yes
