Csaba Ringhofer has posted comments on this change. ( http://gerrit.cloudera.org:8080/20648 )
Change subject: IMPALA-10987: Changing impala.disableHmsSync in Hive should not break event processing ...................................................................... Patch Set 1: (13 comments) http://gerrit.cloudera.org:8080/#/c/20648/1//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/20648/1//COMMIT_MSG@11 PS1, Line 11: n a table The patch only deals with table level disabling of event sync, but the problem also seems relevant if event sync is disabled at db level. I don't think that we need to solve that case in this patch, but it would be good to know what is expected if things change on the db level, and whether tests exist for this case. http://gerrit.cloudera.org:8080/#/c/20648/1//COMMIT_MSG@15 PS1, Line 15: re-enabled does anything change in case when the event sync is disabled via HMS? If not, the the title could also contain re-enabled. http://gerrit.cloudera.org:8080/#/c/20648/1//COMMIT_MSG@17 PS1, Line 17: a) We can just invalidate the table, if the current event is nit: extra whitespace http://gerrit.cloudera.org:8080/#/c/20648/1//COMMIT_MSG@19 PS1, Line 19: the first time query accesses it. shouldn't there be a case b? http://gerrit.cloudera.org:8080/#/c/20648/1//COMMIT_MSG@22 PS1, Line 22: 3 I don't understand this case - why should we see any event where event sync is disabled when it was just re-enabled? Shouldn't we get an alter table event first that disables it again? http://gerrit.cloudera.org:8080/#/c/20648/1//COMMIT_MSG@22 PS1, Line 22: that nit: "where"? http://gerrit.cloudera.org:8080/#/c/20648/1/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java File fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java: http://gerrit.cloudera.org:8080/#/c/20648/1/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java@809 PS1, Line 809: invalidateTable It seems counter-intuitive to do this change in a property like function "isEventProcessingDisabled" Also, if I understand correctly, then this will be always called when the table is loaded and event processing is disabled for the table, not just when the event specifically disables event processing. Doesn't this mean that catalog will always invalidate the table, leading to a lot of reloads if the table is heavily used by coordinators and there are many events? http://gerrit.cloudera.org:8080/#/c/20648/1/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java@1565 PS1, Line 1565: } else if (getEventId() > 0 && getEventId() <= tbl.getCreateEventId()) { : // Older event, so this event will be skipped. : metrics_.getCounter(MetastoreEventsProcessor.EVENTS_SKIPPED_METRIC).inc(); : debugLog("Incremented skipped metric to " + : metrics_.getCounter(MetastoreEventsProcessor.EVENTS_SKIPPED_METRIC) : .getCount()); Shouldn't we handle this in isOlderEvent()? http://gerrit.cloudera.org:8080/#/c/20648/1/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java@1578 PS1, Line 1578: t nit: +2 indentation http://gerrit.cloudera.org:8080/#/c/20648/1/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java@1580 PS1, Line 1580: the table does not exist The error message looks stale. Also, can we really go this branch? If I understand correctly, this should only happen if getEventId < 0, but is that really possible? http://gerrit.cloudera.org:8080/#/c/20648/1/fe/src/test/java/org/apache/impala/catalog/events/MetastoreEventsProcessorTest.java File fe/src/test/java/org/apache/impala/catalog/events/MetastoreEventsProcessorTest.java: http://gerrit.cloudera.org:8080/#/c/20648/1/fe/src/test/java/org/apache/impala/catalog/events/MetastoreEventsProcessorTest.java@1701 PS1, Line 1701: // that the table is not existing in catalog anymore. Event processing shouldnot nit: missing space http://gerrit.cloudera.org:8080/#/c/20648/1/fe/src/test/java/org/apache/impala/catalog/events/MetastoreEventsProcessorTest.java@1742 PS1, Line 1742: ACTIVE Will it be inactive if enableSyncToLatestEventOnDdls() is true? If not then we could always verify this assert. http://gerrit.cloudera.org:8080/#/c/20648/1/fe/src/test/java/org/apache/impala/catalog/events/MetastoreEventsProcessorTest.java@1750 PS1, Line 1750: susbsequent typo -- To view, visit http://gerrit.cloudera.org:8080/20648 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I37055990be49e91462ebc98aa97009ca768a0072 Gerrit-Change-Number: 20648 Gerrit-PatchSet: 1 Gerrit-Owner: Sai Hemanth Gantasala <[email protected]> Gerrit-Reviewer: Csaba Ringhofer <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Quanlong Huang <[email protected]> Gerrit-Reviewer: Sai Hemanth Gantasala <[email protected]> Gerrit-Comment-Date: Thu, 09 Nov 2023 11:25:59 +0000 Gerrit-HasComments: Yes
