You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@impala.apache.org by "Sourabh Goyal (Code Review)" <ge...@cloudera.org> on 2021/07/30 14:23:34 UTC
[Impala-ASF-CR] [WIP]: Add drop event id to deleteEventLog for drop db/table/partition events
Sourabh Goyal has uploaded this change for review. ( http://gerrit.cloudera.org:8080/17741
Change subject: [WIP]: Add drop event id to deleteEventLog for drop db/table/partition events
......................................................................
[WIP]: Add drop event id to deleteEventLog for drop db/table/partition events
Change-Id: Iffa519a477e211891e7fa7118950628ec42b45d4
---
M fe/src/main/java/org/apache/impala/catalog/events/DeleteEventLog.java
M fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java
M fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java
M fe/src/test/java/org/apache/impala/catalog/events/MetastoreEventsProcessorTest.java
4 files changed, 81 insertions(+), 7 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/41/17741/1
--
To view, visit http://gerrit.cloudera.org:8080/17741
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: Iffa519a477e211891e7fa7118950628ec42b45d4
Gerrit-Change-Number: 17741
Gerrit-PatchSet: 1
Gerrit-Owner: Sourabh Goyal <so...@cloudera.com>
[Impala-ASF-CR] [WIP]: Add drop event id to deleteEventLog for drop db/table/partition events
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/17741 )
Change subject: [WIP]: Add drop event id to deleteEventLog for drop db/table/partition events
......................................................................
Patch Set 4:
Build Failed
https://jenkins.impala.io/job/gerrit-code-review-checks/9216/ : Initial code review checks failed. See linked job for details on the failure.
--
To view, visit http://gerrit.cloudera.org:8080/17741
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iffa519a477e211891e7fa7118950628ec42b45d4
Gerrit-Change-Number: 17741
Gerrit-PatchSet: 4
Gerrit-Owner: Sourabh Goyal <so...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Comment-Date: Fri, 30 Jul 2021 18:10:00 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] [WIP]: Add drop event id to deleteEventLog for drop db/table/partition events
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/17741 )
Change subject: [WIP]: Add drop event id to deleteEventLog for drop db/table/partition events
......................................................................
Patch Set 2:
Build Failed
https://jenkins.impala.io/job/gerrit-code-review-checks/9214/ : Initial code review checks failed. See linked job for details on the failure.
--
To view, visit http://gerrit.cloudera.org:8080/17741
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iffa519a477e211891e7fa7118950628ec42b45d4
Gerrit-Change-Number: 17741
Gerrit-PatchSet: 2
Gerrit-Owner: Sourabh Goyal <so...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Comment-Date: Fri, 30 Jul 2021 14:59:26 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] [WIP]: Add drop event id to deleteEventLog for drop db/table/partition events
Posted by "Sourabh Goyal (Code Review)" <ge...@cloudera.org>.
Sourabh Goyal has posted comments on this change. ( http://gerrit.cloudera.org:8080/17741 )
Change subject: [WIP]: Add drop event id to deleteEventLog for drop db/table/partition events
......................................................................
Patch Set 4:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/17741/4/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java
File fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java:
http://gerrit.cloudera.org:8080/#/c/17741/4/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@873
PS4, Line 873: addToDeleteEventLog
> I don't understand this change. Perhaps you can explain me this better.
My thought process behind this change was:
While syncing to latest event ID, when processing drop db/table/partition events, we need to add them to delete entry log. Consider the following sequence of events from Impala shell (assuming we are syncing to latest event id for every DDL operation):
1. at t1 -> create_table
2. at t2 -> drop_table
3. at t3 -> create_table event received by event processor
here t3 > t2 > t1
Now after #2, table would be removed from cache. And since create_table event by event processor should be a no-op, we need to add table to deleteEntryLog after drop_table request.
Since drop event is calling this method, I thought of extending its functionality to also add eventID to deleteEvent log so that from CatalogOpExecutor and MetastoreService handler's code for syncing to latest event id would simply look like
for(MetastoreEvent event : metastoreEvents) {
event.processIfEnabled();
}
What happens when this method is called from EventProcessor - This would add event ID to deleteEntryLog as you rightly pointed out. But note that after every event gets processed by EventProcessor, it also gets garbage collected from deleteEventEntry log. Pasting snippet:
for (MetastoreEvent event : filteredEvents) {
event.processIfEnabled();
deleteEventLog_.garbageCollect(event.getEventId());
lastSyncedEventId_.set(event.eventId_);
}
}
Please let me know if I misunderstood something or you think this is not the right approach.
--
To view, visit http://gerrit.cloudera.org:8080/17741
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iffa519a477e211891e7fa7118950628ec42b45d4
Gerrit-Change-Number: 17741
Gerrit-PatchSet: 4
Gerrit-Owner: Sourabh Goyal <so...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Sourabh Goyal <so...@cloudera.com>
Gerrit-Reviewer: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Comment-Date: Mon, 02 Aug 2021 15:09:13 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] [WIP]: Add drop event id to deleteEventLog for drop db/table/partition events
Posted by "Sourabh Goyal (Code Review)" <ge...@cloudera.org>.
Hello Impala Public Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/17741
to look at the new patch set (#4).
Change subject: [WIP]: Add drop event id to deleteEventLog for drop db/table/partition events
......................................................................
[WIP]: Add drop event id to deleteEventLog for drop db/table/partition events
Change-Id: Iffa519a477e211891e7fa7118950628ec42b45d4
---
M fe/src/main/java/org/apache/impala/catalog/events/DeleteEventLog.java
M fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java
M fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java
M fe/src/test/java/org/apache/impala/catalog/events/MetastoreEventsProcessorTest.java
4 files changed, 127 insertions(+), 7 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/41/17741/4
--
To view, visit http://gerrit.cloudera.org:8080/17741
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Iffa519a477e211891e7fa7118950628ec42b45d4
Gerrit-Change-Number: 17741
Gerrit-PatchSet: 4
Gerrit-Owner: Sourabh Goyal <so...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
[Impala-ASF-CR] [WIP]: Add drop event id to deleteEventLog for drop db/table/partition events
Posted by "Sourabh Goyal (Code Review)" <ge...@cloudera.org>.
Hello Impala Public Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/17741
to look at the new patch set (#3).
Change subject: [WIP]: Add drop event id to deleteEventLog for drop db/table/partition events
......................................................................
[WIP]: Add drop event id to deleteEventLog for drop db/table/partition events
Change-Id: Iffa519a477e211891e7fa7118950628ec42b45d4
---
M fe/src/main/java/org/apache/impala/catalog/events/DeleteEventLog.java
M fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java
M fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java
M fe/src/test/java/org/apache/impala/catalog/events/MetastoreEventsProcessorTest.java
4 files changed, 82 insertions(+), 7 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/41/17741/3
--
To view, visit http://gerrit.cloudera.org:8080/17741
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Iffa519a477e211891e7fa7118950628ec42b45d4
Gerrit-Change-Number: 17741
Gerrit-PatchSet: 3
Gerrit-Owner: Sourabh Goyal <so...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
[Impala-ASF-CR] [WIP]: Add drop event id to deleteEventLog for drop db/table/partition events
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/17741 )
Change subject: [WIP]: Add drop event id to deleteEventLog for drop db/table/partition events
......................................................................
Patch Set 1:
Build Failed
https://jenkins.impala.io/job/gerrit-code-review-checks/9213/ : Initial code review checks failed. See linked job for details on the failure.
--
To view, visit http://gerrit.cloudera.org:8080/17741
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iffa519a477e211891e7fa7118950628ec42b45d4
Gerrit-Change-Number: 17741
Gerrit-PatchSet: 1
Gerrit-Owner: Sourabh Goyal <so...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Comment-Date: Fri, 30 Jul 2021 14:33:56 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] [WIP]: Add drop event id to deleteEventLog for drop db/table/partition events
Posted by "Vihang Karajgaonkar (Code Review)" <ge...@cloudera.org>.
Vihang Karajgaonkar has posted comments on this change. ( http://gerrit.cloudera.org:8080/17741 )
Change subject: [WIP]: Add drop event id to deleteEventLog for drop db/table/partition events
......................................................................
Patch Set 4:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/17741/4/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java
File fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java:
http://gerrit.cloudera.org:8080/#/c/17741/4/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@709
PS4, Line 709: addToDeleteEventLog
If the goal is to make sure that drops from the metastore service handler are added to the deleteEventLog then this should be done at a higher level from like from removeNonTransactionalTableIfExists() method because this method is executed by events processor's drop table event as well.
http://gerrit.cloudera.org:8080/#/c/17741/4/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@873
PS4, Line 873: addToDeleteEventLog
I don't understand this change. Perhaps you can explain me this better.
This method is called from MetastoreEventsProcessor when it processes the DROP_DATABASE event. Why do we need to add it to deleteEventLog for this? The main idea of deleteEventLog is to detect self-events for drop events. The DDL execution logic will add to the delete event log and events processor will remove it when it received that event. But here I see that we are adding it from events processor? How do we even use this delete event added to the log since we are never going to receive the same event again.
--
To view, visit http://gerrit.cloudera.org:8080/17741
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iffa519a477e211891e7fa7118950628ec42b45d4
Gerrit-Change-Number: 17741
Gerrit-PatchSet: 4
Gerrit-Owner: Sourabh Goyal <so...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Comment-Date: Fri, 30 Jul 2021 18:21:57 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] [WIP]: Add drop event id to deleteEventLog for drop db/table/partition events
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/17741 )
Change subject: [WIP]: Add drop event id to deleteEventLog for drop db/table/partition events
......................................................................
Patch Set 3:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/9215/ : 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/17741
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iffa519a477e211891e7fa7118950628ec42b45d4
Gerrit-Change-Number: 17741
Gerrit-PatchSet: 3
Gerrit-Owner: Sourabh Goyal <so...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Comment-Date: Fri, 30 Jul 2021 18:04:15 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] [WIP]: Add drop event id to deleteEventLog for drop db/table/partition events
Posted by "Vihang Karajgaonkar (Code Review)" <ge...@cloudera.org>.
Vihang Karajgaonkar has posted comments on this change. ( http://gerrit.cloudera.org:8080/17741 )
Change subject: [WIP]: Add drop event id to deleteEventLog for drop db/table/partition events
......................................................................
Patch Set 4:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/17741/4/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java
File fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java:
http://gerrit.cloudera.org:8080/#/c/17741/4/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@873
PS4, Line 873: addToDeleteEventLog
> My thought process behind this change was:
Thanks for the explanation. The approach only slightly simplifies the DDL code path in the assumed scenario where DDLs sync to latest event id. e.g.
for(MetastoreEvent event : metastoreEvents) {
event.processIfEnabled();
}
v/s
for(MetastoreEvent event : metastoreEvents) {
event.processIfEnabled();
addToDeleteEventLog(event);
}
While on the flip side, it complicates the events processor path now where we seem to unnecessarily add event in the deleteLog which is anyways going to be garbage collected. This could reduce debuggability of the code. Also, it seems that on the event code path, we are overwriting the same event in the deleteEventLog which is counter intuitive. In my opinion the benefits are not significant enough to outweigh added complexity. Thoughts?
--
To view, visit http://gerrit.cloudera.org:8080/17741
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iffa519a477e211891e7fa7118950628ec42b45d4
Gerrit-Change-Number: 17741
Gerrit-PatchSet: 4
Gerrit-Owner: Sourabh Goyal <so...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Sourabh Goyal <so...@cloudera.com>
Gerrit-Reviewer: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Comment-Date: Mon, 02 Aug 2021 21:04:20 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] [WIP]: Add drop event id to deleteEventLog for drop db/table/partition events
Posted by "Sourabh Goyal (Code Review)" <ge...@cloudera.org>.
Hello Impala Public Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/17741
to look at the new patch set (#2).
Change subject: [WIP]: Add drop event id to deleteEventLog for drop db/table/partition events
......................................................................
[WIP]: Add drop event id to deleteEventLog for drop db/table/partition events
Change-Id: Iffa519a477e211891e7fa7118950628ec42b45d4
---
M fe/src/main/java/org/apache/impala/catalog/events/DeleteEventLog.java
M fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java
M fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java
M fe/src/test/java/org/apache/impala/catalog/events/MetastoreEventsProcessorTest.java
4 files changed, 81 insertions(+), 7 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/41/17741/2
--
To view, visit http://gerrit.cloudera.org:8080/17741
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Iffa519a477e211891e7fa7118950628ec42b45d4
Gerrit-Change-Number: 17741
Gerrit-PatchSet: 2
Gerrit-Owner: Sourabh Goyal <so...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
[Impala-ASF-CR] [WIP]: Add drop event id to deleteEventLog for drop db/table/partition events
Posted by "Sourabh Goyal (Code Review)" <ge...@cloudera.org>.
Sourabh Goyal has posted comments on this change. ( http://gerrit.cloudera.org:8080/17741 )
Change subject: [WIP]: Add drop event id to deleteEventLog for drop db/table/partition events
......................................................................
Patch Set 4:
(1 comment)
> Patch Set 4:
>
> (1 comment)
http://gerrit.cloudera.org:8080/#/c/17741/4/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java
File fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java:
http://gerrit.cloudera.org:8080/#/c/17741/4/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@873
PS4, Line 873: addToDeleteEventLog
> Thanks for the explanation. The approach only slightly simplifies the DDL c
Sure @Vihang. I agree that there is a better way to handle adding event ids to deleteEventLog for Impala shell and Metastore server. I will abandon this patch.
Thank you for sharing your feedback !
--
To view, visit http://gerrit.cloudera.org:8080/17741
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iffa519a477e211891e7fa7118950628ec42b45d4
Gerrit-Change-Number: 17741
Gerrit-PatchSet: 4
Gerrit-Owner: Sourabh Goyal <so...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Sourabh Goyal <so...@cloudera.com>
Gerrit-Reviewer: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Comment-Date: Tue, 03 Aug 2021 10:17:29 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] [WIP]: Add drop event id to deleteEventLog for drop db/table/partition events
Posted by "Sourabh Goyal (Code Review)" <ge...@cloudera.org>.
Sourabh Goyal has abandoned this change. ( http://gerrit.cloudera.org:8080/17741 )
Change subject: [WIP]: Add drop event id to deleteEventLog for drop db/table/partition events
......................................................................
Abandoned
No longer required.
--
To view, visit http://gerrit.cloudera.org:8080/17741
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: abandon
Gerrit-Change-Id: Iffa519a477e211891e7fa7118950628ec42b45d4
Gerrit-Change-Number: 17741
Gerrit-PatchSet: 4
Gerrit-Owner: Sourabh Goyal <so...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Sourabh Goyal <so...@cloudera.com>
Gerrit-Reviewer: Vihang Karajgaonkar <vi...@cloudera.com>