Bharath Vissapragada has posted comments on this change. ( http://gerrit.cloudera.org:8080/13664 )
Change subject: IMPALA-7534. Handle invalidation races in CatalogdMetaProvider ...................................................................... Patch Set 2: (4 comments) Sorry, I got confused by the comments on the jira. It appeared that they are two different issues but upon looking closely, you seem to be fixing that race. Patch makes sense to me. I have some minor comments. http://gerrit.cloudera.org:8080/#/c/13664/2/fe/src/main/java/org/apache/impala/catalog/local/CatalogdMetaProvider.java File fe/src/main/java/org/apache/impala/catalog/local/CatalogdMetaProvider.java: http://gerrit.cloudera.org:8080/#/c/13664/2/fe/src/main/java/org/apache/impala/catalog/local/CatalogdMetaProvider.java@427 PS2, Line 427: private <CacheKeyType, ValueType> ValueType loadWithCaching(String itemString, I think this could use a method comment since the logic is non-trivial (probably move the cache_ comment to this method)? http://gerrit.cloudera.org:8080/#/c/13664/2/fe/src/main/java/org/apache/impala/catalog/local/CatalogdMetaProvider.java@435 PS2, Line 435: Object inCache = cache_.get(key, () -> f); Just for my understanding, here we are relying on the inherent thread-safety of get(key, callable) right? Even if there are concurrent loadWithCaching() calls, only the first future is added to the cache and all other threads block on its get(). http://gerrit.cloudera.org:8080/#/c/13664/2/fe/src/main/java/org/apache/impala/catalog/local/CatalogdMetaProvider.java@573 PS2, Line 573: Uninterruptibles.sleepUninterruptibly(10, TimeUnit.MILLISECONDS); Remove? http://gerrit.cloudera.org:8080/#/c/13664/2/fe/src/main/java/org/apache/impala/catalog/local/CatalogdMetaProvider.java@687 PS2, Line 687: RuntimeException Does this mean the query is not eventually retried? -- To view, visit http://gerrit.cloudera.org:8080/13664 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I70f377db88e204825a909389f28dc3451815235c Gerrit-Change-Number: 13664 Gerrit-PatchSet: 2 Gerrit-Owner: Todd Lipcon <[email protected]> Gerrit-Reviewer: Bharath Vissapragada <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-Reviewer: Vihang Karajgaonkar <[email protected]> Gerrit-Comment-Date: Tue, 18 Jun 2019 18:58:10 +0000 Gerrit-HasComments: Yes
