Quanlong Huang has posted comments on this change. ( http://gerrit.cloudera.org:8080/20853 )
Change subject: IMPALA-12670: getIfPresent should throw the cause of error ...................................................................... Patch Set 3: (4 comments) > Patch Set 2: > > (1 comment) > > I reread the original Guava issue > (https://github.com/google/guava/issues/1881). > Looks like even in Caffeine, invalidateAll still have undefined behavior even > with AsyncLoadingCache. > > There is also a mention about Guava wrapper that forked out from Trino. Maybe > we can consider that to simplify CatalogdMetaProvider cache. > https://github.com/findepi/evictable-cache Thanks for digging into this! Filed IMPALA-12682 to track this topic which needs more investigation. http://gerrit.cloudera.org:8080/#/c/20853/2/be/src/exec/catalog-op-executor.cc File be/src/exec/catalog-op-executor.cc: http://gerrit.cloudera.org:8080/#/c/20853/2/be/src/exec/catalog-op-executor.cc@60 PS2, Line 60: DEFINE_double_hidden(inject_failure_ratio_in_catalog_fetch, -1, > This could be a debug action, but seems like we already have a pattern here Yeah, I think either way is ok. It's a hidden flag and we don't plan to expose this in query options. http://gerrit.cloudera.org:8080/#/c/20853/2/be/src/exec/catalog-op-executor.cc@61 PS2, Line 61: "Ratio to randomly fail the GetPartialCatalogObject RPC with TABLE_NOT_LOADED " > What's the allowed range for this ratio? And why default to -1 rather than Any value is allowed. Negative values disable it. Values larger than 1 always trigger it. I'll update the description. http://gerrit.cloudera.org:8080/#/c/20853/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/20853/2/fe/src/main/java/org/apache/impala/catalog/local/CatalogdMetaProvider.java@899 PS2, Line 899: } catch (ExecutionException e) { UncheckedExecutionException was added in IMPALA-7530/IMPALA-7509 without explanation: https://gerrit.cloudera.org/c/11403/7/fe/src/main/java/org/apache/impala/catalog/local/CatalogdMetaProvider.java#361 The javadoc of Cache.get() shows the same as Riza mentioned: https://github.com/google/guava/blob/0a17f4a429323589396c38d8ce75ca058faa6c64/guava/src/com/google/common/cache/Cache.java#L97-L100 * ExecutionException – if a checked exception was thrown while loading the value * UncheckedExecutionException – if an unchecked exception was thrown while loading the value * ExecutionError – if an error was thrown while loading the value So I think we don't need UncheckedExecutionException here since we don't use the Guava cache in the try-block. > getUninterruptibly() in this try block can throw ExecutionException and > CancellationException (a RuntimeException subclass). The latter will be > wrapped again in line 903? Not sure how the CancellationException will be thrown since we never cancel the work of sendRequest(). But if we do catch it, it will be wrapped in line 903. That's ok since we just want the InconsistentMetadataFetchException being exposed. Other exceptions are not retriable and it's ok to fail the planning. http://gerrit.cloudera.org:8080/#/c/20853/2/tests/custom_cluster/test_local_catalog.py File tests/custom_cluster/test_local_catalog.py: http://gerrit.cloudera.org:8080/#/c/20853/2/tests/custom_cluster/test_local_catalog.py@475 PS2, Line 475: tls.c, "select * from {0}.tbl where p=0".format(unique_database)) > nit: +2 indentation Done -- To view, visit http://gerrit.cloudera.org:8080/20853 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I74268ba2bb700988107780e13ffbdbb4c767d09d Gerrit-Change-Number: 20853 Gerrit-PatchSet: 3 Gerrit-Owner: Quanlong Huang <[email protected]> Gerrit-Reviewer: Csaba Ringhofer <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Michael Smith <[email protected]> Gerrit-Reviewer: Quanlong Huang <[email protected]> Gerrit-Reviewer: Riza Suminto <[email protected]> Gerrit-Comment-Date: Fri, 05 Jan 2024 00:29:33 +0000 Gerrit-HasComments: Yes
