Shant Hovsepian has posted comments on this change. ( http://gerrit.cloudera.org:8080/16549 )
Change subject: IMPALA-6671: [WIP] Skip locked tables from topic updates ...................................................................... Patch Set 1: (6 comments) Thanks for working on this Vihang, it will be a life saver. http://gerrit.cloudera.org:8080/#/c/16549/1/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/16549/1/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@1294 PS1, Line 1294: true A freeform boolean is hard to read, best to add an inline comment , true /* tryLock */ or make it an Enum. http://gerrit.cloudera.org:8080/#/c/16549/1/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@1316 PS1, Line 1316: topicUpdateTblLockMaxWaitTimeMs I could be misunderstanding the code, but isn't 7200 seconds too long to wait before skipping the update? http://gerrit.cloudera.org:8080/#/c/16549/1/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@1325 PS1, Line 1325: return; I hope the updateTblPendingVersion() rarely exceeds the number of attempts, but if it does throw an exception here, wouldn't it be better to try something else. http://gerrit.cloudera.org:8080/#/c/16549/1/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@1352 PS1, Line 1352: if (attempt >= Table.NUMBER_OF_ATTEMPTS_TO_UPDATE_VERSION) { : throw new CatalogException("Exhausted max number of attempts " : + Table.NUMBER_OF_ATTEMPTS_TO_UPDATE_VERSION : + " to update the pending table version"); : } Wouldn't we want to try something more graceful in this case. How would the end user recover from this exception. http://gerrit.cloudera.org:8080/#/c/16549/1/fe/src/main/java/org/apache/impala/catalog/HdfsTable.java File fe/src/main/java/org/apache/impala/catalog/HdfsTable.java: http://gerrit.cloudera.org:8080/#/c/16549/1/fe/src/main/java/org/apache/impala/catalog/HdfsTable.java@2662 PS1, Line 2662: LOG.info(String : .format( : "%s the pending catalog version of table %s from %s to %s%s.", : (success ? "Bumped up" : "Attempt to bump up"), getFullName(), : expectedPendingVersion, : newCatalogVersion, success ? "" : " failed")); LOG Debug? Isn't this too frequent. http://gerrit.cloudera.org:8080/#/c/16549/1/fe/src/main/java/org/apache/impala/catalog/HdfsTable.java@2677 PS1, Line 2677: getAndSet Why set it to -1? I haven't been able to convince myself that setting it -1 wouldn't prevent another caller from setting the catalog version to something older than intended. -- To view, visit http://gerrit.cloudera.org:8080/16549 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ic657b96edbcdc94c6b906e7ca59291f4e4715655 Gerrit-Change-Number: 16549 Gerrit-PatchSet: 1 Gerrit-Owner: Vihang Karajgaonkar <vih...@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Shant Hovsepian <sh...@cloudera.com> Gerrit-Reviewer: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-Reviewer: Vihang Karajgaonkar <vih...@cloudera.com> Gerrit-Comment-Date: Wed, 07 Oct 2020 15:31:36 +0000 Gerrit-HasComments: Yes
