You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@impala.apache.org by "Csaba Ringhofer (Code Review)" <ge...@cloudera.org> on 2023/09/27 19:30:37 UTC
[Impala-ASF-CR] POC IMPALA-12461: Avoid taking db/table level locks db/table self-event check
Csaba Ringhofer has uploaded this change for review. ( http://gerrit.cloudera.org:8080/20516
Change subject: POC IMPALA-12461: Avoid taking db/table level locks db/table self-event check
......................................................................
POC IMPALA-12461: Avoid taking db/table level locks db/table self-event check
DB / Table level locks can be held for a long time by DDL/DML
operations in the catalogd. Trying to get the lock during self-event
check would mean blocking if there is an ongoing operation for
the given db/table.
This patch tries to solve it for the easy case of db/table level
events by using self-event specific locking that should be only
taken for short times to add/remove events from/to the in-flight
event list.
Postponing solving this for partition level events as that would
be more complex, as both the partition list and the partitions'
in-flight event lists would need to be protected from parallel
operations that add/remove partitions.
Testing:
- ran event processing related tests
Change-Id: Ife455de09ab2e262bde1e4b5bd54c8c54c75f2cd
---
M fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java
M fe/src/main/java/org/apache/impala/catalog/Db.java
M fe/src/main/java/org/apache/impala/catalog/HdfsTable.java
M fe/src/main/java/org/apache/impala/catalog/Table.java
4 files changed, 103 insertions(+), 73 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/16/20516/1
--
To view, visit http://gerrit.cloudera.org:8080/20516
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: Ife455de09ab2e262bde1e4b5bd54c8c54c75f2cd
Gerrit-Change-Number: 20516
Gerrit-PatchSet: 1
Gerrit-Owner: Csaba Ringhofer <cs...@cloudera.com>
[Impala-ASF-CR] IMPALA-12461 part1: Avoid taking db/table level locks db/table self-event check
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/20516 )
Change subject: IMPALA-12461 part1: Avoid taking db/table level locks db/table self-event check
......................................................................
Patch Set 3:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/14101/ : 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/20516
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ife455de09ab2e262bde1e4b5bd54c8c54c75f2cd
Gerrit-Change-Number: 20516
Gerrit-PatchSet: 3
Gerrit-Owner: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Thu, 28 Sep 2023 16:26:06 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-12461 part1: Avoid taking db/table level locks db/table self-event check
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/20516 )
Change subject: IMPALA-12461 part1: Avoid taking db/table level locks db/table self-event check
......................................................................
Patch Set 5: Verified+1
--
To view, visit http://gerrit.cloudera.org:8080/20516
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ife455de09ab2e262bde1e4b5bd54c8c54c75f2cd
Gerrit-Change-Number: 20516
Gerrit-PatchSet: 5
Gerrit-Owner: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Fri, 29 Sep 2023 12:29:52 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-12461 part1: Avoid taking db/table level locks db/table self-event check
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/20516 )
Change subject: IMPALA-12461 part1: Avoid taking db/table level locks db/table self-event check
......................................................................
Patch Set 5: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/20516
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ife455de09ab2e262bde1e4b5bd54c8c54c75f2cd
Gerrit-Change-Number: 20516
Gerrit-PatchSet: 5
Gerrit-Owner: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Fri, 29 Sep 2023 08:11:39 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] POC IMPALA-12461: Avoid taking db/table level locks db/table self-event check
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/20516 )
Change subject: POC IMPALA-12461: Avoid taking db/table level locks db/table self-event check
......................................................................
Patch Set 1: Verified-1
Build failed: https://jenkins.impala.io/job/gerrit-verify-dryrun/9767/
--
To view, visit http://gerrit.cloudera.org:8080/20516
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ife455de09ab2e262bde1e4b5bd54c8c54c75f2cd
Gerrit-Change-Number: 20516
Gerrit-PatchSet: 1
Gerrit-Owner: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Comment-Date: Wed, 27 Sep 2023 23:58:00 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-12461 part1: Avoid taking db/table level locks db/table self-event check
Posted by "Csaba Ringhofer (Code Review)" <ge...@cloudera.org>.
Hello Zoltan Borok-Nagy, Impala Public Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/20516
to look at the new patch set (#3).
Change subject: IMPALA-12461 part1: Avoid taking db/table level locks db/table self-event check
......................................................................
IMPALA-12461 part1: Avoid taking db/table level locks db/table self-event check
DB / Table level locks can be held for a long time by DDL/DML
operations in the catalogd. Trying to get the lock during self-event
check would mean blocking if there is an ongoing operation for
the given db/table.
This patch tries to solve it for the easy case of db/table level
events by using self-event specific locking that should be only
taken for short times to add/remove events from/to the in-flight
event list. The InFlightEvents object itself is used as lock
and no other of catalogd's lock must be acquired after it to
avoid deadlock.
Postponing solving this for partition level events as that would
be more complex, as both the partition list and the partitions'
in-flight event lists would need to be protected from parallel
operations that add/remove partitions.
Testing:
- ran event processing related tests
Change-Id: Ife455de09ab2e262bde1e4b5bd54c8c54c75f2cd
---
M fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java
M fe/src/main/java/org/apache/impala/catalog/Db.java
M fe/src/main/java/org/apache/impala/catalog/HdfsTable.java
M fe/src/main/java/org/apache/impala/catalog/Table.java
4 files changed, 114 insertions(+), 80 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/16/20516/3
--
To view, visit http://gerrit.cloudera.org:8080/20516
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ife455de09ab2e262bde1e4b5bd54c8c54c75f2cd
Gerrit-Change-Number: 20516
Gerrit-PatchSet: 3
Gerrit-Owner: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
[Impala-ASF-CR] IMPALA-12461 part1: Avoid taking db/table level locks db/table self-event check
Posted by "Csaba Ringhofer (Code Review)" <ge...@cloudera.org>.
Csaba Ringhofer has posted comments on this change. ( http://gerrit.cloudera.org:8080/20516 )
Change subject: IMPALA-12461 part1: Avoid taking db/table level locks db/table self-event check
......................................................................
Patch Set 4:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/20516/3/fe/src/main/java/org/apache/impala/catalog/Db.java
File fe/src/main/java/org/apache/impala/catalog/Db.java:
http://gerrit.cloudera.org:8080/#/c/20516/3/fe/src/main/java/org/apache/impala/catalog/Db.java@584
PS3, Line 584: safety
> nit: safety
Done
--
To view, visit http://gerrit.cloudera.org:8080/20516
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ife455de09ab2e262bde1e4b5bd54c8c54c75f2cd
Gerrit-Change-Number: 20516
Gerrit-PatchSet: 4
Gerrit-Owner: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Thu, 28 Sep 2023 21:16:20 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-12461 part1: Avoid taking db/table level locks db/table self-event check
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/20516 )
Change subject: IMPALA-12461 part1: Avoid taking db/table level locks db/table self-event check
......................................................................
Patch Set 4:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/14102/ : 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/20516
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ife455de09ab2e262bde1e4b5bd54c8c54c75f2cd
Gerrit-Change-Number: 20516
Gerrit-PatchSet: 4
Gerrit-Owner: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Thu, 28 Sep 2023 16:48:34 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-12461 part1: Avoid taking db/table level locks db/table self-event check
Posted by "Csaba Ringhofer (Code Review)" <ge...@cloudera.org>.
Csaba Ringhofer has posted comments on this change. ( http://gerrit.cloudera.org:8080/20516 )
Change subject: IMPALA-12461 part1: Avoid taking db/table level locks db/table self-event check
......................................................................
Patch Set 3:
(11 comments)
http://gerrit.cloudera.org:8080/#/c/20516/2//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/20516/2//COMMIT_MSG@16
PS2, Line 16: the in-flight
: event list.
> You could mention that the lock is on this object itself, and when it is lo
Done
http://gerrit.cloudera.org:8080/#/c/20516/2/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java
File fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java:
http://gerrit.cloudera.org:8080/#/c/20516/2/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@1054
PS2, Line 1054: // Not taking lock, rely on Db's internal locking.
> Don't we need 'versionLock_.writeLock().unlock();' here?
As we discussed offline, this is not needed.
http://gerrit.cloudera.org:8080/#/c/20516/2/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@1064
PS2, Line 1064: == null
> When it is not null, it is never empty, right? Should we have a preconditio
tbh I don't know, I didn't want to change the existing logic in this regard.
http://gerrit.cloudera.org:8080/#/c/20516/2/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@1065
PS2, Line 1065: // Not taking lock, rely on Table's internal locking.
> Shouldn't we have 'versionLock_.writeLock().unlock();'?
As we discussed offline, this is not needed.
http://gerrit.cloudera.org:8080/#/c/20516/2/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@1076
PS2, Line 1076: :
> nit: could you please create Jira for this?
I used the current Jira and added part 1 to the commit message.
http://gerrit.cloudera.org:8080/#/c/20516/2/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@1078
PS2, Line 1078: ot help
> I don't think we need the word 'default' here
Done
http://gerrit.cloudera.org:8080/#/c/20516/2/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@1078
PS2, Line 1078: timeout
> timeout
Done
http://gerrit.cloudera.org:8080/#/c/20516/2/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@1081
PS2, Line 1081: if (!tryWriteLock(tbl)) {
: throw new CatalogException(String.format("Error during self-event evaluation "
: + "for table %s due to lock contention", tbl.getFullName()));
: }
: versionLock_.writeLock().unlock();
> This, and the whole try-finally stmt could be moved to evaluateSelfEventFor
I kept it there to minimize nesting in evaluateSelfEventForPartition. As that is a more complex function I prefer to keep the noise of try/catch outside.
http://gerrit.cloudera.org:8080/#/c/20516/2/fe/src/main/java/org/apache/impala/catalog/Db.java
File fe/src/main/java/org/apache/impala/catalog/Db.java:
http://gerrit.cloudera.org:8080/#/c/20516/2/fe/src/main/java/org/apache/impala/catalog/Db.java@102
PS2, Line 102: as a monit
> nit: as a monitor object
Done
http://gerrit.cloudera.org:8080/#/c/20516/2/fe/src/main/java/org/apache/impala/catalog/Db.java@583
PS2, Line 583: ightEvents(lon
> nit: thread safety
Done
http://gerrit.cloudera.org:8080/#/c/20516/2/fe/src/main/java/org/apache/impala/catalog/Table.java
File fe/src/main/java/org/apache/impala/catalog/Table.java:
http://gerrit.cloudera.org:8080/#/c/20516/2/fe/src/main/java/org/apache/impala/catalog/Table.java@152
PS2, Line 152: as a monit
> nit: as a monitor object
Done
--
To view, visit http://gerrit.cloudera.org:8080/20516
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ife455de09ab2e262bde1e4b5bd54c8c54c75f2cd
Gerrit-Change-Number: 20516
Gerrit-PatchSet: 3
Gerrit-Owner: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Thu, 28 Sep 2023 16:02:01 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] POC IMPALA-12461: Avoid taking db/table level locks db/table self-event check
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/20516 )
Change subject: POC IMPALA-12461: Avoid taking db/table level locks db/table self-event check
......................................................................
Patch Set 2:
Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/9768/ DRY_RUN=true
--
To view, visit http://gerrit.cloudera.org:8080/20516
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ife455de09ab2e262bde1e4b5bd54c8c54c75f2cd
Gerrit-Change-Number: 20516
Gerrit-PatchSet: 2
Gerrit-Owner: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Comment-Date: Thu, 28 Sep 2023 05:50:06 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-12461 part1: Avoid taking db/table level locks db/table self-event check
Posted by "Zoltan Borok-Nagy (Code Review)" <ge...@cloudera.org>.
Zoltan Borok-Nagy has posted comments on this change. ( http://gerrit.cloudera.org:8080/20516 )
Change subject: IMPALA-12461 part1: Avoid taking db/table level locks db/table self-event check
......................................................................
Patch Set 3: Code-Review+2
(1 comment)
LGTM!
http://gerrit.cloudera.org:8080/#/c/20516/3/fe/src/main/java/org/apache/impala/catalog/Db.java
File fe/src/main/java/org/apache/impala/catalog/Db.java:
http://gerrit.cloudera.org:8080/#/c/20516/3/fe/src/main/java/org/apache/impala/catalog/Db.java@584
PS3, Line 584: safity
nit: safety
--
To view, visit http://gerrit.cloudera.org:8080/20516
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ife455de09ab2e262bde1e4b5bd54c8c54c75f2cd
Gerrit-Change-Number: 20516
Gerrit-PatchSet: 3
Gerrit-Owner: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Thu, 28 Sep 2023 16:16:03 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] POC IMPALA-12461: Avoid taking db/table level locks db/table self-event check
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/20516 )
Change subject: POC IMPALA-12461: Avoid taking db/table level locks db/table self-event check
......................................................................
Patch Set 1:
Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/9767/ DRY_RUN=true
--
To view, visit http://gerrit.cloudera.org:8080/20516
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ife455de09ab2e262bde1e4b5bd54c8c54c75f2cd
Gerrit-Change-Number: 20516
Gerrit-PatchSet: 1
Gerrit-Owner: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Comment-Date: Wed, 27 Sep 2023 19:31:26 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] POC IMPALA-12461: Avoid taking db/table level locks db/table self-event check
Posted by "Csaba Ringhofer (Code Review)" <ge...@cloudera.org>.
Hello Impala Public Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/20516
to look at the new patch set (#2).
Change subject: POC IMPALA-12461: Avoid taking db/table level locks db/table self-event check
......................................................................
POC IMPALA-12461: Avoid taking db/table level locks db/table self-event check
DB / Table level locks can be held for a long time by DDL/DML
operations in the catalogd. Trying to get the lock during self-event
check would mean blocking if there is an ongoing operation for
the given db/table.
This patch tries to solve it for the easy case of db/table level
events by using self-event specific locking that should be only
taken for short times to add/remove events from/to the in-flight
event list.
Postponing solving this for partition level events as that would
be more complex, as both the partition list and the partitions'
in-flight event lists would need to be protected from parallel
operations that add/remove partitions.
Testing:
- ran event processing related tests
Change-Id: Ife455de09ab2e262bde1e4b5bd54c8c54c75f2cd
---
M fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java
M fe/src/main/java/org/apache/impala/catalog/Db.java
M fe/src/main/java/org/apache/impala/catalog/HdfsTable.java
M fe/src/main/java/org/apache/impala/catalog/Table.java
4 files changed, 112 insertions(+), 80 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/16/20516/2
--
To view, visit http://gerrit.cloudera.org:8080/20516
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ife455de09ab2e262bde1e4b5bd54c8c54c75f2cd
Gerrit-Change-Number: 20516
Gerrit-PatchSet: 2
Gerrit-Owner: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
[Impala-ASF-CR] POC IMPALA-12461: Avoid taking db/table level locks db/table self-event check
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/20516 )
Change subject: POC IMPALA-12461: Avoid taking db/table level locks db/table self-event check
......................................................................
Patch Set 1:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/14092/ : 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/20516
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ife455de09ab2e262bde1e4b5bd54c8c54c75f2cd
Gerrit-Change-Number: 20516
Gerrit-PatchSet: 1
Gerrit-Owner: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Comment-Date: Wed, 27 Sep 2023 19:57:19 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-12461 part1: Avoid taking db/table level locks db/table self-event check
Posted by "Csaba Ringhofer (Code Review)" <ge...@cloudera.org>.
Csaba Ringhofer has posted comments on this change. ( http://gerrit.cloudera.org:8080/20516 )
Change subject: IMPALA-12461 part1: Avoid taking db/table level locks db/table self-event check
......................................................................
Patch Set 3:
(note that the last PS only changed comment, so rerunning tests doesn't seem necessary)
--
To view, visit http://gerrit.cloudera.org:8080/20516
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ife455de09ab2e262bde1e4b5bd54c8c54c75f2cd
Gerrit-Change-Number: 20516
Gerrit-PatchSet: 3
Gerrit-Owner: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Thu, 28 Sep 2023 16:02:38 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-12461 part1: Avoid taking db/table level locks db/table self-event check
Posted by "Csaba Ringhofer (Code Review)" <ge...@cloudera.org>.
Hello Zoltan Borok-Nagy, Impala Public Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/20516
to look at the new patch set (#4).
Change subject: IMPALA-12461 part1: Avoid taking db/table level locks db/table self-event check
......................................................................
IMPALA-12461 part1: Avoid taking db/table level locks db/table self-event check
DB / Table level locks can be held for a long time by DDL/DML
operations in the catalogd. Trying to get the lock during self-event
check would mean blocking if there is an ongoing operation for
the given db/table.
This patch tries to solve it for the easy case of db/table level
events by using self-event specific locking that should be only
taken for short times to add/remove events from/to the in-flight
event list. The InFlightEvents object itself is used as lock
and no other of catalogd's lock must be acquired after it to
avoid deadlock.
Postponing solving this for partition level events as that would
be more complex, as both the partition list and the partitions'
in-flight event lists would need to be protected from parallel
operations that add/remove partitions.
Testing:
- ran event processing related tests
Change-Id: Ife455de09ab2e262bde1e4b5bd54c8c54c75f2cd
---
M fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java
M fe/src/main/java/org/apache/impala/catalog/Db.java
M fe/src/main/java/org/apache/impala/catalog/HdfsTable.java
M fe/src/main/java/org/apache/impala/catalog/Table.java
4 files changed, 114 insertions(+), 80 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/16/20516/4
--
To view, visit http://gerrit.cloudera.org:8080/20516
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ife455de09ab2e262bde1e4b5bd54c8c54c75f2cd
Gerrit-Change-Number: 20516
Gerrit-PatchSet: 4
Gerrit-Owner: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
[Impala-ASF-CR] IMPALA-12461 part1: Avoid taking db/table level locks db/table self-event check
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/20516 )
Change subject: IMPALA-12461 part1: Avoid taking db/table level locks db/table self-event check
......................................................................
IMPALA-12461 part1: Avoid taking db/table level locks db/table self-event check
DB / Table level locks can be held for a long time by DDL/DML
operations in the catalogd. Trying to get the lock during self-event
check would mean blocking if there is an ongoing operation for
the given db/table.
This patch tries to solve it for the easy case of db/table level
events by using self-event specific locking that should be only
taken for short times to add/remove events from/to the in-flight
event list. The InFlightEvents object itself is used as lock
and no other of catalogd's lock must be acquired after it to
avoid deadlock.
Postponing solving this for partition level events as that would
be more complex, as both the partition list and the partitions'
in-flight event lists would need to be protected from parallel
operations that add/remove partitions.
Testing:
- ran event processing related tests
Change-Id: Ife455de09ab2e262bde1e4b5bd54c8c54c75f2cd
Reviewed-on: http://gerrit.cloudera.org:8080/20516
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/CatalogServiceCatalog.java
M fe/src/main/java/org/apache/impala/catalog/Db.java
M fe/src/main/java/org/apache/impala/catalog/HdfsTable.java
M fe/src/main/java/org/apache/impala/catalog/Table.java
4 files changed, 114 insertions(+), 80 deletions(-)
Approvals:
Impala Public Jenkins: Looks good to me, approved; Verified
--
To view, visit http://gerrit.cloudera.org:8080/20516
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: Ife455de09ab2e262bde1e4b5bd54c8c54c75f2cd
Gerrit-Change-Number: 20516
Gerrit-PatchSet: 6
Gerrit-Owner: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
[Impala-ASF-CR] POC IMPALA-12461: Avoid taking db/table level locks db/table self-event check
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/20516 )
Change subject: POC IMPALA-12461: Avoid taking db/table level locks db/table self-event check
......................................................................
Patch Set 2: Verified+1
--
To view, visit http://gerrit.cloudera.org:8080/20516
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ife455de09ab2e262bde1e4b5bd54c8c54c75f2cd
Gerrit-Change-Number: 20516
Gerrit-PatchSet: 2
Gerrit-Owner: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Comment-Date: Thu, 28 Sep 2023 10:19:46 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] POC IMPALA-12461: Avoid taking db/table level locks db/table self-event check
Posted by "Zoltan Borok-Nagy (Code Review)" <ge...@cloudera.org>.
Zoltan Borok-Nagy has posted comments on this change. ( http://gerrit.cloudera.org:8080/20516 )
Change subject: POC IMPALA-12461: Avoid taking db/table level locks db/table self-event check
......................................................................
Patch Set 2:
(11 comments)
Nice change!
http://gerrit.cloudera.org:8080/#/c/20516/2//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/20516/2//COMMIT_MSG@16
PS2, Line 16: the in-flight
: event list.
You could mention that the lock is on this object itself, and when it is locked no other locks are taken, i.e. we cannot deadlock.
http://gerrit.cloudera.org:8080/#/c/20516/2/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java
File fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java:
http://gerrit.cloudera.org:8080/#/c/20516/2/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@1054
PS2, Line 1054: // Not taking lock, rely on Db's internal locking.
Don't we need 'versionLock_.writeLock().unlock();' here?
http://gerrit.cloudera.org:8080/#/c/20516/2/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@1064
PS2, Line 1064: == null
When it is not null, it is never empty, right? Should we have a precondition check for this after this if stmt?
http://gerrit.cloudera.org:8080/#/c/20516/2/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@1065
PS2, Line 1065: // Not taking lock, rely on Table's internal locking.
Shouldn't we have 'versionLock_.writeLock().unlock();'?
http://gerrit.cloudera.org:8080/#/c/20516/2/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@1076
PS2, Line 1076: :
nit: could you please create Jira for this?
http://gerrit.cloudera.org:8080/#/c/20516/2/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@1078
PS2, Line 1078: time out
timeout
http://gerrit.cloudera.org:8080/#/c/20516/2/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@1078
PS2, Line 1078: default
I don't think we need the word 'default' here
http://gerrit.cloudera.org:8080/#/c/20516/2/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@1081
PS2, Line 1081: if (!tryWriteLock(tbl)) {
: throw new CatalogException(String.format("Error during self-event evaluation "
: + "for table %s due to lock contention", tbl.getFullName()));
: }
: versionLock_.writeLock().unlock();
This, and the whole try-finally stmt could be moved to evaluateSelfEventForPartition()
http://gerrit.cloudera.org:8080/#/c/20516/2/fe/src/main/java/org/apache/impala/catalog/Db.java
File fe/src/main/java/org/apache/impala/catalog/Db.java:
http://gerrit.cloudera.org:8080/#/c/20516/2/fe/src/main/java/org/apache/impala/catalog/Db.java@102
PS2, Line 102: as monitor
nit: as a monitor object
http://gerrit.cloudera.org:8080/#/c/20516/2/fe/src/main/java/org/apache/impala/catalog/Db.java@583
PS2, Line 583: thready safity
nit: thread safety
http://gerrit.cloudera.org:8080/#/c/20516/2/fe/src/main/java/org/apache/impala/catalog/Table.java
File fe/src/main/java/org/apache/impala/catalog/Table.java:
http://gerrit.cloudera.org:8080/#/c/20516/2/fe/src/main/java/org/apache/impala/catalog/Table.java@152
PS2, Line 152: as monitor
nit: as a monitor object
--
To view, visit http://gerrit.cloudera.org:8080/20516
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ife455de09ab2e262bde1e4b5bd54c8c54c75f2cd
Gerrit-Change-Number: 20516
Gerrit-PatchSet: 2
Gerrit-Owner: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Thu, 28 Sep 2023 15:18:28 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-12461 part1: Avoid taking db/table level locks db/table self-event check
Posted by "Zoltan Borok-Nagy (Code Review)" <ge...@cloudera.org>.
Zoltan Borok-Nagy has posted comments on this change. ( http://gerrit.cloudera.org:8080/20516 )
Change subject: IMPALA-12461 part1: Avoid taking db/table level locks db/table self-event check
......................................................................
Patch Set 4: Code-Review+2
LGTM!
--
To view, visit http://gerrit.cloudera.org:8080/20516
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ife455de09ab2e262bde1e4b5bd54c8c54c75f2cd
Gerrit-Change-Number: 20516
Gerrit-PatchSet: 4
Gerrit-Owner: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Fri, 29 Sep 2023 08:11:10 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-12461 part1: Avoid taking db/table level locks db/table self-event check
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/20516 )
Change subject: IMPALA-12461 part1: Avoid taking db/table level locks db/table self-event check
......................................................................
Patch Set 5:
Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/9777/ DRY_RUN=false
--
To view, visit http://gerrit.cloudera.org:8080/20516
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ife455de09ab2e262bde1e4b5bd54c8c54c75f2cd
Gerrit-Change-Number: 20516
Gerrit-PatchSet: 5
Gerrit-Owner: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Fri, 29 Sep 2023 08:11:40 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] POC IMPALA-12461: Avoid taking db/table level locks db/table self-event check
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/20516 )
Change subject: POC IMPALA-12461: Avoid taking db/table level locks db/table self-event check
......................................................................
Patch Set 2:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/14095/ : 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/20516
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ife455de09ab2e262bde1e4b5bd54c8c54c75f2cd
Gerrit-Change-Number: 20516
Gerrit-PatchSet: 2
Gerrit-Owner: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Comment-Date: Thu, 28 Sep 2023 06:15:55 +0000
Gerrit-HasComments: No