Alex Behm has posted comments on this change. Change subject: IMPALA-4449: Revisit table locking pattern in the catalog ......................................................................
Patch Set 1: (10 comments) http://gerrit.cloudera.org:8080/#/c/5710/1/fe/src/main/java/org/apache/impala/catalog/Table.java File fe/src/main/java/org/apache/impala/catalog/Table.java: Line 72: // Lock protecting this table // Fair lock protecting this table. Why not a ReentrantLock? 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 172: * consistently in the presence of concurrent DDL operations. This pattern ensures that This -> The following? Line 175: * prevent starvation from happening. remove "from happening" Line 182: * CONTINUE Looks like we might busy spin for a long time. Consider adding a wait. Line 183: * } ELSE { remove ELSE block to simplify? Line 357: while (true) { Comment why we are looping forever. Do you think it makes sense to consider a very conservative timeout for the overall operation? It seems a little scary to have a thread busy spinning forever. If we get into a pathological/buggy scenario, restarting the catalogd may be the only remedy. Line 359: if (tbl.getLock().writeLock().tryLock()) { Reverse the logic like in your snippet in the class comment. Line 361: if (params.getAlter_type() == TAlterTableType.RENAME_VIEW Can we move this inner logic into a separate function? It would be nice to see the locking and retry logic in a small snippet of code. Line 652: if (tbl.getLock().writeLock().tryLock()) { same here with reversing the logic Line 1456: while (true) { Instead of replicating the lock crabbing+retry logic in several places, can you think of a reasonable way to abstract it out? Just asking for a discussion, maybe it doesn't make sense. -- 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 <[email protected]> Gerrit-Reviewer: Alex Behm <[email protected]> Gerrit-Reviewer: Bharath Vissapragada <[email protected]> Gerrit-HasComments: Yes
