Sai Hemanth Gantasala 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 2: (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 prob DB level enable/disable worked fine when I tested. Let me upload the test for the DB level in the next patch set. 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 no When event sync is disabled from HMS or Impala has the same effect. The events for that table are no longer processed. 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 greater > nit: extra whitespace Ack http://gerrit.cloudera.org:8080/#/c/20648/1//COMMIT_MSG@19 PS1, Line 19: time query accesses it. > shouldn't there be a case b? I thought it was obvious. I have updated it the commit message anyway. http://gerrit.cloudera.org:8080/#/c/20648/1//COMMIT_MSG@22 PS1, Line 22: te l > nit: "where"? Ack http://gerrit.cloudera.org:8080/#/c/20648/1//COMMIT_MSG@22 PS1, Line 22: t > I don't understand this case - why should we see any event where event sync Apologies for the confusion. This shouldn't be under the re-enable scenario. Let me move this to a different section. Consider that the eventSync is disabled on a table, and after that, the table is loaded/cached in Impala. If the event processor sees any events for that table, we don't process those events since eventSync is disabled. Now, if we were to access this table from Impala, we would access the older table. So for this reason, if we see any event of the table (with event sync disabled) and the table is loaded/cached, then we can invalidate the table so that the next access can fetch the latest snapshot. 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 "i Agree, I can move this newly added code to a different function and call the function here. but I believe this is the only place we can do this. Regarding why we need to do this, I tried to explain in the other comment. Please let me know if it is unclear. 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()? Agree!! 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 Ack 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 und I was reluctant to add this else section, but just added this to make sure we are not ignoring the unwarranted state. 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 Ack 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 Let's verify the eventProcessor status always!! 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 Ack -- 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: 2 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: Wed, 29 Nov 2023 03:24:50 +0000 Gerrit-HasComments: Yes
