Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/16549 )
Change subject: IMPALA-6671: Skip locked tables from topic updates ...................................................................... Patch Set 6: (6 comments) I was able to do a pass over it, but didn't quite understand the stuff with pending versions yet and my brain is running out of steam tonight. Maybe you can help me understand it and I can take another look in the morning? http://gerrit.cloudera.org:8080/#/c/16549/6/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/6/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@1304 PS6, Line 1304: topicUpdateEntry.getNumSkippedUpdatesLockContention() : == maxSkippedUpdatesLockContention) Maybe use a local variable - canSkipUpdate? The relationship between this condition and topicUpdateEntry.getNumSkippedUpdatesLockContention() < maxSkippedUpdatesLockContention is a little hard to pick up otherwise. http://gerrit.cloudera.org:8080/#/c/16549/6/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@1339 PS6, Line 1339: hdfstable nit: hdfsTable http://gerrit.cloudera.org:8080/#/c/16549/6/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@1349 PS6, Line 1349: if (hdfstable.getLastVersionSeenByTopicUpdate() != topicUpdateEntry Just to make sure I understand correctly, in the case when hdfstable.getLastVersionSeenByTopicUpdate() == topicUpdateEntry .getLastSentVersion() then we don't need to update topicUpdateLog() because we don't need to update the counter of skipped updates and the previous entry still has all the correct data? http://gerrit.cloudera.org:8080/#/c/16549/6/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@1394 PS6, Line 1394: // Currently, there could be only 2 threads which can concurrently update the I'm not currently quite grokking the need to have all the bits of this synchronization scheme and it would be nice to avoid the retry loop so there's no failure mode to think about (I also need to check that the CatalogException is handled cleanly). My understanding is that we're bumping the pending version when it skips an update so that when the long-running operation finishes and setCatalogVersion is called, the version will be high enough to be picked up by a later topic update. I didn't understand why CatalogOpExecutor needs to update the pending version when it's already allocating and updating a catalog version. http://gerrit.cloudera.org:8080/#/c/16549/6/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@1397 PS6, Line 1397: atmost nit: at most http://gerrit.cloudera.org:8080/#/c/16549/6/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@1398 PS6, Line 1398: e nit: extra e -- 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: 6 Gerrit-Owner: Vihang Karajgaonkar <vih...@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Quanlong Huang <huangquanl...@gmail.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: Thu, 22 Oct 2020 05:57:57 +0000 Gerrit-HasComments: Yes
