Dimitris Tsirogiannis has posted comments on this change. Change subject: IMPALA-4449: Revisit table locking pattern in the catalog ......................................................................
Patch Set 1: (17 comments) http://gerrit.cloudera.org:8080/#/c/5710/1//COMMIT_MSG Commit Message: PS1, Line 29: getAllCatalogObjects > getCatalogObjects()? Done 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 wa Added a trace message for now. Line 928: if (tbl.getLock().writeLock().tryLock()) { > To avoid extra branch, may be we could refactor to something like Done PS1, Line 950: continue; > redundant? Yes, removed. Line 1305: hdfsTable.setCatalogVersion(newCatalogVersion); > - Just curious, is this an existing bug that we didn't set it if the partit Hm, why do we need to update the table version if the partition already exists? Line 1312: continue; > redundant? Yes, removed. 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. Done 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? Done Line 175: * prevent starvation from happening. > remove "from happening" Done Line 182: * CONTINUE > Looks like we might busy spin for a long time. Consider adding a wait. Done Line 183: * } ELSE { > remove ELSE block to simplify? Done Line 190: * } > I think we should document the exception of getCatalogObjects() here. Done Line 357: while (true) { > Comment why we are looping forever. Added an 1hr timeout. Let me know if you believe a larger one is required. Line 359: if (tbl.getLock().writeLock().tryLock()) { > Reverse the logic like in your snippet in the class comment. Done Line 361: if (params.getAlter_type() == TAlterTableType.RENAME_VIEW > Can we move this inner logic into a separate function? It would be nice to Done Line 652: if (tbl.getLock().writeLock().tryLock()) { > same here with reversing the logic Done Line 1456: while (true) { > Instead of replicating the lock crabbing+retry logic in several places, can Done -- 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 <dtsirogian...@cloudera.com> Gerrit-Reviewer: Alex Behm <alex.b...@cloudera.com> Gerrit-Reviewer: Bharath Vissapragada <bhara...@cloudera.com> Gerrit-Reviewer: Dimitris Tsirogiannis <dtsirogian...@cloudera.com> Gerrit-HasComments: Yes
