You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@impala.apache.org by "Sai Hemanth Gantasala (Code Review)" <ge...@cloudera.org> on 2023/11/03 00:48:57 UTC
[Impala-ASF-CR] IMPALA-10987: Changing impala.disableHmsSync in Hive should not break event processing
Sai Hemanth Gantasala has uploaded this change for review. ( http://gerrit.cloudera.org:8080/20648
Change subject: IMPALA-10987: Changing impala.disableHmsSync in Hive should not break event processing
......................................................................
IMPALA-10987: Changing impala.disableHmsSync in
Hive should not break event processing
Currently we require a global invalidate to reset the events processor
if the events sync is re-enabled on a table from HMS. This patch
eliminates the need to reset the catalog cache when events sync is
re-enabled.
Implementation details: when events sync is re-enabled on table via HMS
1) If the table exists in Impala,
a) We can just invalidate the table, if the current event is
greater than the create event id of the table, so that it is reloaded
the first time query accesses it.
2) If the table doesn't exist in Impala, create a Incomplete table, if
there is no entry in the event delete log for this table.
3) For all subsequent table events that eventSync is disabled, if the
table object is loaded, mark the table as stale, so that it is reloaded
the next time query accesses it.
Testing:
1) manually verified few scenarios.
2) Added test case for the above scenarios.
Change-Id: I37055990be49e91462ebc98aa97009ca768a0072
---
M fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java
M fe/src/test/java/org/apache/impala/catalog/events/MetastoreEventsProcessorTest.java
2 files changed, 106 insertions(+), 36 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/48/20648/1
--
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: newchange
Gerrit-Change-Id: I37055990be49e91462ebc98aa97009ca768a0072
Gerrit-Change-Number: 20648
Gerrit-PatchSet: 1
Gerrit-Owner: Sai Hemanth Gantasala <sa...@cloudera.com>
[Impala-ASF-CR] IMPALA-10987: Changing impala.disableHmsSync in Hive should not break event processing
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins 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:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/14540/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.
--
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 <sa...@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: Sai Hemanth Gantasala <sa...@cloudera.com>
Gerrit-Comment-Date: Wed, 29 Nov 2023 03:51:12 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-10987: Changing impala.disableHmsSync in Hive should not break event processing
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins 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 5:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/14630/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.
--
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: 5
Gerrit-Owner: Sai Hemanth Gantasala <sa...@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: Sai Hemanth Gantasala <sa...@cloudera.com>
Gerrit-Comment-Date: Fri, 08 Dec 2023 23:08:57 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-10987: Changing impala.disableHmsSync in Hive should not break event processing
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins 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 9:
Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/10006/ DRY_RUN=true
--
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: 9
Gerrit-Owner: Sai Hemanth Gantasala <sa...@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: Sai Hemanth Gantasala <sa...@cloudera.com>
Gerrit-Comment-Date: Tue, 12 Dec 2023 19:14:51 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-10987: Changing impala.disableHmsSync in Hive should not break event processing
Posted by "Quanlong Huang (Code Review)" <ge...@cloudera.org>.
Quanlong Huang 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 9: Code-Review+2
--
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: 9
Gerrit-Owner: Sai Hemanth Gantasala <sa...@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: Sai Hemanth Gantasala <sa...@cloudera.com>
Gerrit-Comment-Date: Wed, 13 Dec 2023 00:34:57 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-10987: Changing impala.disableHmsSync in Hive should not break event processing
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins 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 10: Verified-1
Build failed: https://jenkins.impala.io/job/gerrit-verify-dryrun/10013/
--
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: 10
Gerrit-Owner: Sai Hemanth Gantasala <sa...@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: Sai Hemanth Gantasala <sa...@cloudera.com>
Gerrit-Comment-Date: Wed, 13 Dec 2023 05:05:06 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-10987: Changing impala.disableHmsSync in Hive should not break event processing
Posted by "Sai Hemanth Gantasala (Code Review)" <ge...@cloudera.org>.
Hello Quanlong Huang, Csaba Ringhofer, Impala Public Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/20648
to look at the new patch set (#7).
Change subject: IMPALA-10987: Changing impala.disableHmsSync in Hive should not break event processing
......................................................................
IMPALA-10987: Changing impala.disableHmsSync in
Hive should not break event processing
Currently we require a global invalidate to reset the events processor
if the events sync is re-enabled on a table from HMS. This patch
eliminates the need to reset the catalog cache when events sync is
re-enabled.
Implementation details: when events sync is re-enabled on table via HMS
1) If the table exists in Impala,
a) We can just invalidate the table, if the current event is greater
than the create event id of the table, so that it is reloaded the first
time query accesses it.
b) Otherwise we can just ignore the event.
2) If the table doesn't exist in Impala, create a Incomplete table, if
there is no entry in the event delete log for this table.
Note: If the eventSync is disabled on a table, for all subsequent table
events, ideally we should mark the table as stale if the table object
is loaded, so that it is reloaded the next time query accesses it. But,
since this approach has performance impact, the events will be ignored.
Testing:
1) manually verified few scenarios.
2) Added test case for the above scenarios.
Change-Id: I37055990be49e91462ebc98aa97009ca768a0072
---
M fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java
M fe/src/test/java/org/apache/impala/catalog/events/MetastoreEventsProcessorTest.java
M tests/custom_cluster/test_events_custom_configs.py
3 files changed, 162 insertions(+), 59 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/48/20648/7
--
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: newpatchset
Gerrit-Change-Id: I37055990be49e91462ebc98aa97009ca768a0072
Gerrit-Change-Number: 20648
Gerrit-PatchSet: 7
Gerrit-Owner: Sai Hemanth Gantasala <sa...@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: Sai Hemanth Gantasala <sa...@cloudera.com>
[Impala-ASF-CR] IMPALA-10987: Changing impala.disableHmsSync in Hive should not break event processing
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins 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 4:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/14618/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.
--
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: 4
Gerrit-Owner: Sai Hemanth Gantasala <sa...@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: Sai Hemanth Gantasala <sa...@cloudera.com>
Gerrit-Comment-Date: Fri, 08 Dec 2023 03:01:21 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-10987: Changing impala.disableHmsSync in Hive should not break event processing
Posted by "Csaba Ringhofer (Code Review)" <ge...@cloudera.org>.
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 5: Code-Review+1
(1 comment)
http://gerrit.cloudera.org:8080/#/c/20648/1//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/20648/1//COMMIT_MSG@22
PS1, Line 22: te l
> Completely agree with your argument. Makes sense to me. I'll ignore the eve
I don't think that a warning is needed, it is enough to log skipping the event at INFO level. IF hms sync is disabled, is is done on purpose.
--
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: 5
Gerrit-Owner: Sai Hemanth Gantasala <sa...@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: Sai Hemanth Gantasala <sa...@cloudera.com>
Gerrit-Comment-Date: Sat, 09 Dec 2023 11:39:03 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-10987: Changing impala.disableHmsSync in Hive should not break event processing
Posted by "Quanlong Huang (Code Review)" <ge...@cloudera.org>.
Quanlong Huang 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 5:
(5 comments)
http://gerrit.cloudera.org:8080/#/c/20648/5/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/5/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java@1535
PS5, Line 1535: // in case of table level alters from external systems it is better to do a full
: // refresh eg. this could be due to as simple as adding a new parameter or a
: // full blown adding or changing column type
: // rename is already handled above
nit: move this comment to L1543 ?
http://gerrit.cloudera.org:8080/#/c/20648/5/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java@1560
PS5, Line 1560: as
nit: "is" ?
http://gerrit.cloudera.org:8080/#/c/20648/5/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java@1563
PS5, Line 1563: if (catalogOpExecutor_.addTableIfNotRemovedLater(getEventId(), msTbl_)) {
If the db is just missing in the cache but exists in HMS, catalogOpExecutor_.addTableIfNotRemovedLater() throws DatabaseNotFoundException. However, we should add the db and the table if they actually exist in HMS.
Can we add a following test case?
* Create a new db in Hive so the db doesn't exist in catalogd's cache.
* Create a new table with event sync disabled in Hive under this db. So the table is also missing in catalogd's cache.
* Alter this table to enable event sync in Hive.
When processing the ALTER_TABLE event, catalogd should not go into the error state and should be able to add the db with the table into the cache.
http://gerrit.cloudera.org:8080/#/c/20648/5/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java@1578
PS5, Line 1578: is turned on this table
nit: "is turned on for this table"
http://gerrit.cloudera.org:8080/#/c/20648/5/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java@1583
PS5, Line 1583: the table does not exist
I think the table exists at this point but its createEventId >= this event id. This event should be skipped by isOlderEvent(). If we go here, it seems a bug. We should add the createEventId in the error message.
--
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: 5
Gerrit-Owner: Sai Hemanth Gantasala <sa...@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: Sai Hemanth Gantasala <sa...@cloudera.com>
Gerrit-Comment-Date: Mon, 11 Dec 2023 12:45:27 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-10987: Changing impala.disableHmsSync in Hive should not break event processing
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins 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 8:
Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/10000/ DRY_RUN=false
--
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: 8
Gerrit-Owner: Sai Hemanth Gantasala <sa...@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: Sai Hemanth Gantasala <sa...@cloudera.com>
Gerrit-Comment-Date: Tue, 12 Dec 2023 06:22:44 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-10987: Changing impala.disableHmsSync in Hive should not break event processing
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins 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 8: Verified-1
Build failed: https://jenkins.impala.io/job/gerrit-verify-dryrun/10000/
--
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: 8
Gerrit-Owner: Sai Hemanth Gantasala <sa...@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: Sai Hemanth Gantasala <sa...@cloudera.com>
Gerrit-Comment-Date: Tue, 12 Dec 2023 10:50:09 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-10987: Changing impala.disableHmsSync in Hive should not break event processing
Posted by "Sai Hemanth Gantasala (Code Review)" <ge...@cloudera.org>.
Hello Quanlong Huang, Csaba Ringhofer, Impala Public Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/20648
to look at the new patch set (#2).
Change subject: IMPALA-10987: Changing impala.disableHmsSync in Hive should not break event processing
......................................................................
IMPALA-10987: Changing impala.disableHmsSync in
Hive should not break event processing
Currently we require a global invalidate to reset the events processor
if the events sync is re-enabled on a table from HMS. This patch
eliminates the need to reset the catalog cache when events sync is
re-enabled.
Implementation details: when events sync is re-enabled on table via HMS
1) If the table exists in Impala,
a) We can just invalidate the table, if the current event is greater
than the create event id of the table, so that it is reloaded the first
time query accesses it.
b) Otherwise we can just ignore the event.
2) If the table doesn't exist in Impala, create a Incomplete table, if
there is no entry in the event delete log for this table.
Note: If the eventSync is disabled on a table, for all subsequent table
events, if the table object is loaded, mark the table as stale, so that
it is reloaded the next time query accesses it.
Testing:
1) manually verified few scenarios.
2) Added test case for the above scenarios.
Change-Id: I37055990be49e91462ebc98aa97009ca768a0072
---
M fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java
M fe/src/test/java/org/apache/impala/catalog/events/MetastoreEventsProcessorTest.java
2 files changed, 106 insertions(+), 36 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/48/20648/2
--
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: newpatchset
Gerrit-Change-Id: I37055990be49e91462ebc98aa97009ca768a0072
Gerrit-Change-Number: 20648
Gerrit-PatchSet: 2
Gerrit-Owner: Sai Hemanth Gantasala <sa...@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: Sai Hemanth Gantasala <sa...@cloudera.com>
[Impala-ASF-CR] IMPALA-10987: Changing impala.disableHmsSync in Hive should not break event processing
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/20648 )
Change subject: IMPALA-10987: Changing impala.disableHmsSync in Hive should not break event processing
......................................................................
IMPALA-10987: Changing impala.disableHmsSync in
Hive should not break event processing
Currently we require a global invalidate to reset the events processor
if the events sync is re-enabled on a table from HMS. This patch
eliminates the need to reset the catalog cache when events sync is
re-enabled.
Implementation details: when events sync is re-enabled on table via HMS
1) If the table exists in Impala,
a) We can just invalidate the table, if the current event is greater
than the create event id of the table, so that it is reloaded the first
time query accesses it.
b) Otherwise we can just ignore the event.
2) If the table doesn't exist in Impala, create a Incomplete table, if
there is no entry in the event delete log for this table.
Note: If the eventSync is disabled on a table, for all subsequent table
events, ideally we should mark the table as stale if the table object
is loaded, so that it is reloaded the next time query accesses it. But,
since this approach has performance impact, the events will be ignored.
Testing:
1) manually verified few scenarios.
2) Added test case for the above scenarios.
Change-Id: I37055990be49e91462ebc98aa97009ca768a0072
Reviewed-on: http://gerrit.cloudera.org:8080/20648
Reviewed-by: Impala Public Jenkins <im...@cloudera.com>
Tested-by: Impala Public Jenkins <im...@cloudera.com>
---
M fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java
M fe/src/test/java/org/apache/impala/catalog/events/MetastoreEventsProcessorTest.java
M tests/custom_cluster/test_events_custom_configs.py
3 files changed, 162 insertions(+), 59 deletions(-)
Approvals:
Impala Public Jenkins: Looks good to me, approved; Verified
--
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: merged
Gerrit-Change-Id: I37055990be49e91462ebc98aa97009ca768a0072
Gerrit-Change-Number: 20648
Gerrit-PatchSet: 12
Gerrit-Owner: Sai Hemanth Gantasala <sa...@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: Sai Hemanth Gantasala <sa...@cloudera.com>
[Impala-ASF-CR] IMPALA-10987: Changing impala.disableHmsSync in Hive should not break event processing
Posted by "Csaba Ringhofer (Code Review)" <ge...@cloudera.org>.
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 3:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/20648/3/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/3/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java@1556
PS3, Line 1556: wasEventSyncTurnedOn
Wouldn't it be better to move this block earlier, before self event check?
I am unsure about the handling of enabling metadata sync as self event. Assume the following scenario:
0. event processing starts disabled for a table
1. another component changes a random table prop with ALTER TABLE
2. before processing the event from 1 impala enables hms sync on the table
3. the event from 1 arrives, but catalogd skips it as hms sync was still disabled at that point
4. the event from 2 arrives, but it is skipped at it is a self event
I think that at 4 catalogd should mark the table as stale. as it didn't process event from 1
http://gerrit.cloudera.org:8080/#/c/20648/3/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java@1557
PS3, Line 1557: // check if the table exists or not. 1) if the table doesn't exist create an
I think that moving this block to a separate function like handleEventSyncTurnedOn() would make the main path clearer.
http://gerrit.cloudera.org:8080/#/c/20648/3/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java@1562
PS3, Line 1562: getTable
Are we prepared for throwing an exception here? Using getTableNoThrow seems more fitting to avoid exception when the DB also doesn't exist (not sure if this is possible at this point)
--
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: 3
Gerrit-Owner: Sai Hemanth Gantasala <sa...@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: Sai Hemanth Gantasala <sa...@cloudera.com>
Gerrit-Comment-Date: Thu, 07 Dec 2023 15:41:35 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-10987: Changing impala.disableHmsSync in Hive should not break event processing
Posted by "Sai Hemanth Gantasala (Code Review)" <ge...@cloudera.org>.
Hello Quanlong Huang, Csaba Ringhofer, Impala Public Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/20648
to look at the new patch set (#6).
Change subject: IMPALA-10987: Changing impala.disableHmsSync in Hive should not break event processing
......................................................................
IMPALA-10987: Changing impala.disableHmsSync in
Hive should not break event processing
Currently we require a global invalidate to reset the events processor
if the events sync is re-enabled on a table from HMS. This patch
eliminates the need to reset the catalog cache when events sync is
re-enabled.
Implementation details: when events sync is re-enabled on table via HMS
1) If the table exists in Impala,
a) We can just invalidate the table, if the current event is greater
than the create event id of the table, so that it is reloaded the first
time query accesses it.
b) Otherwise we can just ignore the event.
2) If the table doesn't exist in Impala, create a Incomplete table, if
there is no entry in the event delete log for this table.
Note: If the eventSync is disabled on a table, for all subsequent table
events, ideally we should mark the table as stale if the table object
is loaded, so that it is reloaded the next time query accesses it. But,
since this approach has performance impact, the events will be ignored.
Testing:
1) manually verified few scenarios.
2) Added test case for the above scenarios.
Change-Id: I37055990be49e91462ebc98aa97009ca768a0072
---
M fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java
M fe/src/test/java/org/apache/impala/catalog/events/MetastoreEventsProcessorTest.java
M tests/custom_cluster/test_events_custom_configs.py
3 files changed, 162 insertions(+), 59 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/48/20648/6
--
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: newpatchset
Gerrit-Change-Id: I37055990be49e91462ebc98aa97009ca768a0072
Gerrit-Change-Number: 20648
Gerrit-PatchSet: 6
Gerrit-Owner: Sai Hemanth Gantasala <sa...@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: Sai Hemanth Gantasala <sa...@cloudera.com>
[Impala-ASF-CR] IMPALA-10987: Changing impala.disableHmsSync in Hive should not break event processing
Posted by "Quanlong Huang (Code Review)" <ge...@cloudera.org>.
Quanlong Huang 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 7:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/20648/7/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/7/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java@1583
PS7, Line 1583: will be skipped
nit: "should have been skipped"
isOlderEvent() is executed before this method. It should have detected this event as stale event and skipped it.
--
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: 7
Gerrit-Owner: Sai Hemanth Gantasala <sa...@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: Sai Hemanth Gantasala <sa...@cloudera.com>
Gerrit-Comment-Date: Tue, 12 Dec 2023 02:29:32 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-10987: Changing impala.disableHmsSync in Hive should not break event processing
Posted by "Quanlong Huang (Code Review)" <ge...@cloudera.org>.
Quanlong Huang 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 8: Code-Review+2
--
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: 8
Gerrit-Owner: Sai Hemanth Gantasala <sa...@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: Sai Hemanth Gantasala <sa...@cloudera.com>
Gerrit-Comment-Date: Tue, 12 Dec 2023 06:22:15 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-10987: Changing impala.disableHmsSync in Hive should not break event processing
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins 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 3:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/14541/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.
--
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: 3
Gerrit-Owner: Sai Hemanth Gantasala <sa...@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: Sai Hemanth Gantasala <sa...@cloudera.com>
Gerrit-Comment-Date: Wed, 29 Nov 2023 03:52:31 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-10987: Changing impala.disableHmsSync in Hive should not break event processing
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins 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 11:
Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/10018/ DRY_RUN=false
--
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: 11
Gerrit-Owner: Sai Hemanth Gantasala <sa...@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: Sai Hemanth Gantasala <sa...@cloudera.com>
Gerrit-Comment-Date: Wed, 13 Dec 2023 08:07:24 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-10987: Changing impala.disableHmsSync in Hive should not break event processing
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins 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 11: Verified+1
--
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: 11
Gerrit-Owner: Sai Hemanth Gantasala <sa...@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: Sai Hemanth Gantasala <sa...@cloudera.com>
Gerrit-Comment-Date: Wed, 13 Dec 2023 12:34:42 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-10987: Changing impala.disableHmsSync in Hive should not break event processing
Posted by "Sai Hemanth Gantasala (Code Review)" <ge...@cloudera.org>.
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 <sa...@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: Sai Hemanth Gantasala <sa...@cloudera.com>
Gerrit-Comment-Date: Wed, 29 Nov 2023 03:24:50 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-10987: Changing impala.disableHmsSync in Hive should not break event processing
Posted by "Sai Hemanth Gantasala (Code Review)" <ge...@cloudera.org>.
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 4:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/20648/3/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/3/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java@1556
PS3, Line 1556: wasEventSyncTurnedOn
> Wouldn't it be better to move this block earlier, before self event check?
In the above scenario, first (2) is executed, and while reloading table metadata it'll fetch altered table property from HMS.
At (3), if the table is loaded, it'll mark the table as stale (See L#809). When (4) arrives, inflight versions will not have (2) since the table was marked as stale in the previous step, so the event is processed and marked that event sync is enabled.
If there is no external event before (4), then the event would have been skipped as self-event.
I have added an end-to-end test to mimic this scenario.
http://gerrit.cloudera.org:8080/#/c/20648/3/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java@1557
PS3, Line 1557: handleEventSyncTurnedOn();
> I think that moving this block to a separate function like handleEventSyncT
Ack
http://gerrit.cloudera.org:8080/#/c/20648/3/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java@1562
PS3, Line 1562: oad.
> Are we prepared for throwing an exception here? Using getTableNoThrow seems
Good point. will implement your suggestion
--
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: 4
Gerrit-Owner: Sai Hemanth Gantasala <sa...@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: Sai Hemanth Gantasala <sa...@cloudera.com>
Gerrit-Comment-Date: Fri, 08 Dec 2023 02:22:20 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-10987: Changing impala.disableHmsSync in Hive should not break event processing
Posted by "Sai Hemanth Gantasala (Code Review)" <ge...@cloudera.org>.
Hello Quanlong Huang, Csaba Ringhofer, Impala Public Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/20648
to look at the new patch set (#5).
Change subject: IMPALA-10987: Changing impala.disableHmsSync in Hive should not break event processing
......................................................................
IMPALA-10987: Changing impala.disableHmsSync in
Hive should not break event processing
Currently we require a global invalidate to reset the events processor
if the events sync is re-enabled on a table from HMS. This patch
eliminates the need to reset the catalog cache when events sync is
re-enabled.
Implementation details: when events sync is re-enabled on table via HMS
1) If the table exists in Impala,
a) We can just invalidate the table, if the current event is greater
than the create event id of the table, so that it is reloaded the first
time query accesses it.
b) Otherwise we can just ignore the event.
2) If the table doesn't exist in Impala, create a Incomplete table, if
there is no entry in the event delete log for this table.
Note: If the eventSync is disabled on a table, for all subsequent table
events, ideally we should mark the table as stale if the table object
is loaded, so that it is reloaded the next time query accesses it. But,
since this approach has performance impact, the events will be ignored.
Testing:
1) manually verified few scenarios.
2) Added test case for the above scenarios.
Change-Id: I37055990be49e91462ebc98aa97009ca768a0072
---
M fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java
M fe/src/test/java/org/apache/impala/catalog/events/MetastoreEventsProcessorTest.java
M tests/custom_cluster/test_events_custom_configs.py
3 files changed, 141 insertions(+), 55 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/48/20648/5
--
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: newpatchset
Gerrit-Change-Id: I37055990be49e91462ebc98aa97009ca768a0072
Gerrit-Change-Number: 20648
Gerrit-PatchSet: 5
Gerrit-Owner: Sai Hemanth Gantasala <sa...@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: Sai Hemanth Gantasala <sa...@cloudera.com>
[Impala-ASF-CR] IMPALA-10987: Changing impala.disableHmsSync in Hive should not break event processing
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins 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 8:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/14655/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.
--
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: 8
Gerrit-Owner: Sai Hemanth Gantasala <sa...@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: Sai Hemanth Gantasala <sa...@cloudera.com>
Gerrit-Comment-Date: Tue, 12 Dec 2023 05:37:04 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-10987: Changing impala.disableHmsSync in Hive should not break event processing
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins 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 6:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/14647/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.
--
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: 6
Gerrit-Owner: Sai Hemanth Gantasala <sa...@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: Sai Hemanth Gantasala <sa...@cloudera.com>
Gerrit-Comment-Date: Mon, 11 Dec 2023 23:34:44 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-10987: Changing impala.disableHmsSync in Hive should not break event processing
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins 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 10: Code-Review+2
--
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: 10
Gerrit-Owner: Sai Hemanth Gantasala <sa...@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: Sai Hemanth Gantasala <sa...@cloudera.com>
Gerrit-Comment-Date: Wed, 13 Dec 2023 00:35:33 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-10987: Changing impala.disableHmsSync in Hive should not break event processing
Posted by "Sai Hemanth Gantasala (Code Review)" <ge...@cloudera.org>.
Hello Quanlong Huang, Csaba Ringhofer, Impala Public Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/20648
to look at the new patch set (#9).
Change subject: IMPALA-10987: Changing impala.disableHmsSync in Hive should not break event processing
......................................................................
IMPALA-10987: Changing impala.disableHmsSync in
Hive should not break event processing
Currently we require a global invalidate to reset the events processor
if the events sync is re-enabled on a table from HMS. This patch
eliminates the need to reset the catalog cache when events sync is
re-enabled.
Implementation details: when events sync is re-enabled on table via HMS
1) If the table exists in Impala,
a) We can just invalidate the table, if the current event is greater
than the create event id of the table, so that it is reloaded the first
time query accesses it.
b) Otherwise we can just ignore the event.
2) If the table doesn't exist in Impala, create a Incomplete table, if
there is no entry in the event delete log for this table.
Note: If the eventSync is disabled on a table, for all subsequent table
events, ideally we should mark the table as stale if the table object
is loaded, so that it is reloaded the next time query accesses it. But,
since this approach has performance impact, the events will be ignored.
Testing:
1) manually verified few scenarios.
2) Added test case for the above scenarios.
Change-Id: I37055990be49e91462ebc98aa97009ca768a0072
---
M fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java
M fe/src/test/java/org/apache/impala/catalog/events/MetastoreEventsProcessorTest.java
M tests/custom_cluster/test_events_custom_configs.py
3 files changed, 162 insertions(+), 59 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/48/20648/9
--
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: newpatchset
Gerrit-Change-Id: I37055990be49e91462ebc98aa97009ca768a0072
Gerrit-Change-Number: 20648
Gerrit-PatchSet: 9
Gerrit-Owner: Sai Hemanth Gantasala <sa...@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: Sai Hemanth Gantasala <sa...@cloudera.com>
[Impala-ASF-CR] IMPALA-10987: Changing impala.disableHmsSync in Hive should not break event processing
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins 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 11: Code-Review+2
--
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: 11
Gerrit-Owner: Sai Hemanth Gantasala <sa...@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: Sai Hemanth Gantasala <sa...@cloudera.com>
Gerrit-Comment-Date: Wed, 13 Dec 2023 08:07:23 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-10987: Changing impala.disableHmsSync in Hive should not break event processing
Posted by "Csaba Ringhofer (Code Review)" <ge...@cloudera.org>.
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 <sa...@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: Sai Hemanth Gantasala <sa...@cloudera.com>
Gerrit-Comment-Date: Thu, 09 Nov 2023 11:25:59 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-10987: Changing impala.disableHmsSync in Hive should not break event processing
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins 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 7:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/14652/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.
--
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: 7
Gerrit-Owner: Sai Hemanth Gantasala <sa...@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: Sai Hemanth Gantasala <sa...@cloudera.com>
Gerrit-Comment-Date: Tue, 12 Dec 2023 02:08:30 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-10987: Changing impala.disableHmsSync in Hive should not break event processing
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins 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 9:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/14673/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.
--
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: 9
Gerrit-Owner: Sai Hemanth Gantasala <sa...@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: Sai Hemanth Gantasala <sa...@cloudera.com>
Gerrit-Comment-Date: Tue, 12 Dec 2023 18:57:55 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-10987: Changing impala.disableHmsSync in Hive should not break event processing
Posted by "Csaba Ringhofer (Code Review)" <ge...@cloudera.org>.
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 4:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/20648/1//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/20648/1//COMMIT_MSG@22
PS1, Line 22: te l
> Ack
jumping back to this topic:
My impression is that this goes against what disabling event sync tries to achieve. I see 2 reasons for disabling it:
1. performance (avoiding event processing to be blocked due to reloads triggered by events)
2. wanting old behavior (table metadata snapshots can be manually controlled by IM/REFRESH)
Invalidating the table goes against both 1 and 2.
If the table is actively queried by Impala and modified by some other component at the same time, then this will lead to constantly invalidating and reloading it.
>Now, if we were to access this table from Impala, we would access the older table
I think that this is the feature here, not a bug :)
So while I agree with invalidating the table in the reenable case, if hms sync is disabled, then we should totally ignore the event. If the user wants to have fresh metadata, then they should keep HMS sync enabled, while if they
http://gerrit.cloudera.org:8080/#/c/20648/3/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/3/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java@1556
PS3, Line 1556: wasEventSyncTurnedOn
> In the above scenario, first (2) is executed, and while reloading table met
ah, I see, we will see 1 anyway as:
- 2's DDL leads to reloading the table
- if the table is invalidated + loaded after 2, the new load will see the fresh state
http://gerrit.cloudera.org:8080/#/c/20648/4/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/4/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java@808
PS4, Line 808: instanceof HdfsTable
What should happen with HBase/Kudu tables? We just want to ignore incomplete tables, right?
--
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: 4
Gerrit-Owner: Sai Hemanth Gantasala <sa...@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: Sai Hemanth Gantasala <sa...@cloudera.com>
Gerrit-Comment-Date: Fri, 08 Dec 2023 10:09:15 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-10987: Changing impala.disableHmsSync in Hive should not break event processing
Posted by "Sai Hemanth Gantasala (Code Review)" <ge...@cloudera.org>.
Hello Quanlong Huang, Csaba Ringhofer, Impala Public Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/20648
to look at the new patch set (#4).
Change subject: IMPALA-10987: Changing impala.disableHmsSync in Hive should not break event processing
......................................................................
IMPALA-10987: Changing impala.disableHmsSync in
Hive should not break event processing
Currently we require a global invalidate to reset the events processor
if the events sync is re-enabled on a table from HMS. This patch
eliminates the need to reset the catalog cache when events sync is
re-enabled.
Implementation details: when events sync is re-enabled on table via HMS
1) If the table exists in Impala,
a) We can just invalidate the table, if the current event is greater
than the create event id of the table, so that it is reloaded the first
time query accesses it.
b) Otherwise we can just ignore the event.
2) If the table doesn't exist in Impala, create a Incomplete table, if
there is no entry in the event delete log for this table.
Note: If the eventSync is disabled on a table, for all subsequent table
events, if the table object is loaded, mark the table as stale, so that
it is reloaded the next time query accesses it.
Testing:
1) manually verified few scenarios.
2) Added test case for the above scenarios.
Change-Id: I37055990be49e91462ebc98aa97009ca768a0072
---
M fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java
M fe/src/test/java/org/apache/impala/catalog/events/MetastoreEventsProcessorTest.java
M tests/custom_cluster/test_events_custom_configs.py
3 files changed, 157 insertions(+), 55 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/48/20648/4
--
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: newpatchset
Gerrit-Change-Id: I37055990be49e91462ebc98aa97009ca768a0072
Gerrit-Change-Number: 20648
Gerrit-PatchSet: 4
Gerrit-Owner: Sai Hemanth Gantasala <sa...@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: Sai Hemanth Gantasala <sa...@cloudera.com>
[Impala-ASF-CR] IMPALA-10987: Changing impala.disableHmsSync in Hive should not break event processing
Posted by "Sai Hemanth Gantasala (Code Review)" <ge...@cloudera.org>.
Hello Quanlong Huang, Csaba Ringhofer, Impala Public Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/20648
to look at the new patch set (#3).
Change subject: IMPALA-10987: Changing impala.disableHmsSync in Hive should not break event processing
......................................................................
IMPALA-10987: Changing impala.disableHmsSync in
Hive should not break event processing
Currently we require a global invalidate to reset the events processor
if the events sync is re-enabled on a table from HMS. This patch
eliminates the need to reset the catalog cache when events sync is
re-enabled.
Implementation details: when events sync is re-enabled on table via HMS
1) If the table exists in Impala,
a) We can just invalidate the table, if the current event is greater
than the create event id of the table, so that it is reloaded the first
time query accesses it.
b) Otherwise we can just ignore the event.
2) If the table doesn't exist in Impala, create a Incomplete table, if
there is no entry in the event delete log for this table.
Note: If the eventSync is disabled on a table, for all subsequent table
events, if the table object is loaded, mark the table as stale, so that
it is reloaded the next time query accesses it.
Testing:
1) manually verified few scenarios.
2) Added test case for the above scenarios.
Change-Id: I37055990be49e91462ebc98aa97009ca768a0072
---
M fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java
M fe/src/test/java/org/apache/impala/catalog/events/MetastoreEventsProcessorTest.java
2 files changed, 126 insertions(+), 49 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/48/20648/3
--
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: newpatchset
Gerrit-Change-Id: I37055990be49e91462ebc98aa97009ca768a0072
Gerrit-Change-Number: 20648
Gerrit-PatchSet: 3
Gerrit-Owner: Sai Hemanth Gantasala <sa...@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: Sai Hemanth Gantasala <sa...@cloudera.com>
[Impala-ASF-CR] IMPALA-10987: Changing impala.disableHmsSync in Hive should not break event processing
Posted by "Sai Hemanth Gantasala (Code Review)" <ge...@cloudera.org>.
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 4:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/20648/1//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/20648/1//COMMIT_MSG@22
PS1, Line 22: te l
> jumping back to this topic:
Completely agree with your argument. Makes sense to me. I'll ignore the event instead of invalidating the metadata object.
Should we throw a LOG.warn() saying the next access to this metadata object might be stale so issue IM/RT to fetch the latest snapshot or should I leave it as it is since a lot of events on the disabled table might flood the log with trivial warning?
http://gerrit.cloudera.org:8080/#/c/20648/3/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/3/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java@1556
PS3, Line 1556: wasEventSyncTurnedOn
> ah, I see, we will see 1 anyway as:
Yeah, that's correct.
http://gerrit.cloudera.org:8080/#/c/20648/4/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/4/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java@808
PS4, Line 808: instanceof HdfsTable
> What should happen with HBase/Kudu tables? We just want to ignore incomplet
We can ignore the event if the table is incomplete. But I'm planning to remove this part based on your other comment.
--
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: 4
Gerrit-Owner: Sai Hemanth Gantasala <sa...@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: Sai Hemanth Gantasala <sa...@cloudera.com>
Gerrit-Comment-Date: Fri, 08 Dec 2023 22:42:18 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-10987: Changing impala.disableHmsSync in Hive should not break event processing
Posted by "Sai Hemanth Gantasala (Code Review)" <ge...@cloudera.org>.
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 6:
(6 comments)
http://gerrit.cloudera.org:8080/#/c/20648/1//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/20648/1//COMMIT_MSG@22
PS1, Line 22: te l
> I don't think that a warning is needed, it is enough to log skipping the ev
Ack
http://gerrit.cloudera.org:8080/#/c/20648/5/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/5/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java@1535
PS5, Line 1535: long startNs = System.nanoTime();
: if (wasEventSyncTurnedOn()) {
: handleEventSyncTurnedOn();
: } else {
> nit: move this comment to L1543 ?
Ack
http://gerrit.cloudera.org:8080/#/c/20648/5/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java@1560
PS5, Line 1560: is
> nit: "is" ?
Ack
http://gerrit.cloudera.org:8080/#/c/20648/5/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java@1563
PS5, Line 1563: if (catalogOpExecutor_.addTableIfNotRemovedLater(getEventId(), msTbl_)) {
> If the db is just missing in the cache but exists in HMS, catalogOpExecutor
I have added a test case for the above scenario. Since we process the create db event before the alter table event in the queue we will not have the event processor going into an error state.
http://gerrit.cloudera.org:8080/#/c/20648/5/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java@1578
PS5, Line 1578: is turned on for this t
> nit: "is turned on for this table"
Ack
http://gerrit.cloudera.org:8080/#/c/20648/5/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java@1583
PS5, Line 1583: the table with createEve
> I think the table exists at this point but its createEventId >= this event
Yeah, this else condition is really not required. I just wanted to make the if-else condition complete.
I'll log the createEventId in the error message.
--
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: 6
Gerrit-Owner: Sai Hemanth Gantasala <sa...@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: Sai Hemanth Gantasala <sa...@cloudera.com>
Gerrit-Comment-Date: Mon, 11 Dec 2023 23:07:40 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-10987: Changing impala.disableHmsSync in Hive should not break event processing
Posted by "Quanlong Huang (Code Review)" <ge...@cloudera.org>.
Quanlong Huang 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 6: Code-Review+1
(3 comments)
http://gerrit.cloudera.org:8080/#/c/20648/5/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/5/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java@1563
PS5, Line 1563: if (catalogOpExecutor_.addTableIfNotRemovedLater(getEventId(), msTbl_)) {
> I have added a test case for the above scenario. Since we process the creat
Ah, I see. Just realized we will always apply ADD_DATABASE event even if the dbproperties have 'impala.disableHmsSync'='true'.
http://gerrit.cloudera.org:8080/#/c/20648/6/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/6/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java@1540
PS6, Line 1540:
nit: add a comma ","
http://gerrit.cloudera.org:8080/#/c/20648/6/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java@1583
PS6, Line 1583: does not exist.
nit: can we remove "does not exist"? It's misleading. I think we should mention the event should be skipped as stale events.
--
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: 6
Gerrit-Owner: Sai Hemanth Gantasala <sa...@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: Sai Hemanth Gantasala <sa...@cloudera.com>
Gerrit-Comment-Date: Tue, 12 Dec 2023 00:27:50 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-10987: Changing impala.disableHmsSync in Hive should not break event processing
Posted by "Sai Hemanth Gantasala (Code Review)" <ge...@cloudera.org>.
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 7:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/20648/5/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/5/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java@1563
PS5, Line 1563: if (catalogOpExecutor_.addTableIfNotRemovedLater(getEventId(), msTbl_)) {
> Ah, I see. Just realized we will always apply ADD_DATABASE event even if th
Ack
http://gerrit.cloudera.org:8080/#/c/20648/6/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/6/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java@1540
PS6, Line 1540: ,
> nit: add a comma ","
Ack
http://gerrit.cloudera.org:8080/#/c/20648/6/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java@1583
PS6, Line 1583: ll be skipped a
> nit: can we remove "does not exist"? It's misleading. I think we should men
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: 7
Gerrit-Owner: Sai Hemanth Gantasala <sa...@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: Sai Hemanth Gantasala <sa...@cloudera.com>
Gerrit-Comment-Date: Tue, 12 Dec 2023 01:41:05 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-10987: Changing impala.disableHmsSync in Hive should not break event processing
Posted by "Sai Hemanth Gantasala (Code Review)" <ge...@cloudera.org>.
Hello Quanlong Huang, Csaba Ringhofer, Impala Public Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/20648
to look at the new patch set (#8).
Change subject: IMPALA-10987: Changing impala.disableHmsSync in Hive should not break event processing
......................................................................
IMPALA-10987: Changing impala.disableHmsSync in
Hive should not break event processing
Currently we require a global invalidate to reset the events processor
if the events sync is re-enabled on a table from HMS. This patch
eliminates the need to reset the catalog cache when events sync is
re-enabled.
Implementation details: when events sync is re-enabled on table via HMS
1) If the table exists in Impala,
a) We can just invalidate the table, if the current event is greater
than the create event id of the table, so that it is reloaded the first
time query accesses it.
b) Otherwise we can just ignore the event.
2) If the table doesn't exist in Impala, create a Incomplete table, if
there is no entry in the event delete log for this table.
Note: If the eventSync is disabled on a table, for all subsequent table
events, ideally we should mark the table as stale if the table object
is loaded, so that it is reloaded the next time query accesses it. But,
since this approach has performance impact, the events will be ignored.
Testing:
1) manually verified few scenarios.
2) Added test case for the above scenarios.
Change-Id: I37055990be49e91462ebc98aa97009ca768a0072
---
M fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java
M fe/src/test/java/org/apache/impala/catalog/events/MetastoreEventsProcessorTest.java
M tests/custom_cluster/test_events_custom_configs.py
3 files changed, 162 insertions(+), 59 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/48/20648/8
--
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: newpatchset
Gerrit-Change-Id: I37055990be49e91462ebc98aa97009ca768a0072
Gerrit-Change-Number: 20648
Gerrit-PatchSet: 8
Gerrit-Owner: Sai Hemanth Gantasala <sa...@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: Sai Hemanth Gantasala <sa...@cloudera.com>
[Impala-ASF-CR] IMPALA-10987: Changing impala.disableHmsSync in Hive should not break event processing
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins 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 9: Verified+1
--
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: 9
Gerrit-Owner: Sai Hemanth Gantasala <sa...@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: Sai Hemanth Gantasala <sa...@cloudera.com>
Gerrit-Comment-Date: Tue, 12 Dec 2023 23:54:58 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-10987: Changing impala.disableHmsSync in Hive should not break event processing
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins 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 10:
Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/10013/ DRY_RUN=false
--
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: 10
Gerrit-Owner: Sai Hemanth Gantasala <sa...@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: Sai Hemanth Gantasala <sa...@cloudera.com>
Gerrit-Comment-Date: Wed, 13 Dec 2023 00:35:34 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-10987: Changing impala.disableHmsSync in Hive should not break event processing
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins 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:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/14315/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.
--
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 <sa...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Comment-Date: Fri, 03 Nov 2023 01:16:57 +0000
Gerrit-HasComments: No