impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Bharath Vissapragada (Code Review)" <ger...@cloudera.org>
Subject [Impala-ASF-CR] IMPALA-4449: Revisit table locking pattern in the catalog
Date Tue, 17 Jan 2017 19:54:11 GMT
Bharath Vissapragada has posted comments on this change.

Change subject: IMPALA-4449: Revisit table locking pattern in the catalog
......................................................................


Patch Set 1:

(7 comments)

http://gerrit.cloudera.org:8080/#/c/5710/1//COMMIT_MSG
Commit Message:

PS1, Line 29: getAllCatalogObjects
getCatalogObjects()?


http://gerrit.cloudera.org:8080/#/c/5710/1/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java
File fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java:

Line 926:     while (true) {
Regarding supportability, should we log a debug message/expose a average wait time metric
via JMX if its stuck in these loops for longer than a configured value? IMO that makes it
easier to debug queries hung at Catalog operations. (Same comment for other methods that follow
this pattern, just wanted to hear you thoughts)


Line 928:       if (tbl.getLock().writeLock().tryLock()) {
To avoid extra branch, may be we could refactor to something like

catalogLock_.writeLock().lock();
if (!tbl.getLock().writeLock().tryLock()) {
  catalogLock_.writeLock().unlock();
  continue;
}
.
.
.
(Same comment for other places that uses this pattern)


PS1, Line 950: continue;
redundant?


Line 1305:           hdfsTable.setCatalogVersion(newCatalogVersion);
- Just curious, is this an existing bug that we didn't set it if the partition exists?

- May be we can move this line after L1285 before the try block since we are setting for both
cases anyway and remove it at L1296?


Line 1312:         continue;
redundant?


http://gerrit.cloudera.org:8080/#/c/5710/1/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java
File fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java:

Line 190:  * }
I think we should document the exception of getCatalogObjects() here.


-- 
To view, visit http://gerrit.cloudera.org:8080/5710
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Id08e21da31deb1f003b3cada4517651f3b3b2bb2
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Dimitris Tsirogiannis <dtsirogiannis@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bharathv@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message