You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@impala.apache.org by "Quanlong Huang (Code Review)" <ge...@cloudera.org> on 2024/03/05 02:05:38 UTC
[Impala-ASF-CR] IMPALA-12543: Detect self-events before finishing DDL
Quanlong Huang has posted comments on this change. ( http://gerrit.cloudera.org:8080/21029 )
Change subject: IMPALA-12543: Detect self-events before finishing DDL
......................................................................
Patch Set 7:
(8 comments)
Thanks for fixing this! It's a great work that requires detailed reviews. Left some comments first. I'll look deeper into the patch.
http://gerrit.cloudera.org:8080/#/c/21029/7/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/21029/7/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEventsProcessor.java@909
PS7, Line 909: LOG.info("Received " + response.getEvents().size() + " events. First event id : "
: + (response.getEvents().size() > 0 ? response.getEvents().get(0).getEventId() :
: "none")
: + ".");
nit: It'd be more readable to use the message template:
LOG.info("Received {} events. First event id: {}.", response.getEvents().size(),
(response.getEvents().size() > 0 ? response.getEvents().get(0).getEventId() :
"none"));
http://gerrit.cloudera.org:8080/#/c/21029/5/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java
File fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java:
http://gerrit.cloudera.org:8080/#/c/21029/5/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@1683
PS5, Line 1683:
> I don't think that in-flight events are needed here. As for a view reloadin
+1. Could you add a comment for this?
http://gerrit.cloudera.org:8080/#/c/21029/7/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java
File fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java:
http://gerrit.cloudera.org:8080/#/c/21029/7/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@1066
PS7, Line 1066: * Register the new version number into table's in-flight events.
Could you add a comment for when should we use this, e.g. before invoking the HMS API for non-insert events?
http://gerrit.cloudera.org:8080/#/c/21029/7/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@1111
PS7, Line 1111: Preconditions.checkState(!table_.hasInProgressModification());
: Preconditions.checkState(!inflightEventRegistered_);
nit: please add error messages for these
http://gerrit.cloudera.org:8080/#/c/21029/7/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@1366
PS7, Line 1366: Preconditions.checkState(reloadMetadata);
nit: let's add an error message for Preconditions.
http://gerrit.cloudera.org:8080/#/c/21029/7/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@4493
PS7, Line 4493: modification.registerInflightEvent();
Might be an existing issue, but should we still do this when 'addedHmsPartitions' is empty, i.e. no partitions are added?
http://gerrit.cloudera.org:8080/#/c/21029/7/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@5819
PS7, Line 5819: private void alterViewSetTblProperties(Table tbl,
Just realized this is invoked in alterTable() instead of alterView().. In alterView(), we don't deal with the inflight versions (at L1754). Maybe we should make them consistent. But not a big deal since reload on views should be fast, unless it's a materialized view (IIRC, we can't alter MV yet).
http://gerrit.cloudera.org:8080/#/c/21029/7/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@6135
PS7, Line 6135: modification.registerInflightEvent();
Should we do this before L6106 where the events will be generated, and handle the failure at L6128?
--
To view, visit http://gerrit.cloudera.org:8080/21029
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I8365c934349ad21a4d9327fc11594d2fc3445f79
Gerrit-Change-Number: 21029
Gerrit-PatchSet: 7
Gerrit-Owner: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Sai Hemanth Gantasala <sa...@cloudera.com>
Gerrit-Comment-Date: Tue, 05 Mar 2024 02:05:38 +0000
Gerrit-HasComments: Yes