Core: Deprecate ContentCache.invalidateAll#10494
Conversation
|
BTW i did a lot of work on google/guava#1881 problem in Trino. |
c12502c to
1567b6e
Compare
|
Hi @findepi, thank you for looking into this. google/guava#1881 also came to my attention recently in an Impala code review. I don't mind marking Can you also review if iceberg/core/src/main/java/org/apache/iceberg/ManifestFiles.java Lines 83 to 87 in 74cf697 |
|
@rizaon thank you for your feedback!
Good point, I also thought about this. The ContentCache class is designed as generic purpose cache of file contents. Given immutability, we shouldn't need invalidation at all.
Thanks for asking! I don't see a problem with that method |
|
Thank you @rizaon @jbonofre @ajantha-bhat for your review! @amogh-jahagirdar @jackye1995 can you please take a look? |
|
@amogh-jahagirdar please take another look, thanks! |
|
BTW Trino challenges around cache invalidation led me to believe this topic is complex and deserving a presentation. there was one at the Trino summit 2023 (slides). |
This method does only best-effort invalidation and is susceptible to a race condition. If the caller changed the state that could be cached (perhaps files on the storage) and calls this method, there is no guarantee that the cache will not contain stale entries some time after this method returns. This is a similar problem as the one described at google/guava#1881. `ContentCache` doesn't use a Guava Cache, it uses Caffeine. Caffeine offers partial solution to this issue, but not for `invalidateAll` call. To avoid accidental incorrect use of ContentCache, deprecate the `invalidateAll` method, which can be deceptive for the caller and remove it later.
|
@findepi This seems like a great find, is there anything we need to do right now though? I just want to make sure we aren't leaving open any issues by only marking it as deprecated? |
|
thank you @RussellSpitzer for your review. |
| } | ||
|
|
||
| /** | ||
| * @deprecated since 1.6.0, will be removed in 1.7.0; This method does only best-effort |
There was a problem hiding this comment.
is this deprecation message still accurate? Since it says it was deprecated in 1.6.0 but actually it was deprecated with 1.7.0
* Deprecate ContentCache.invalidateAll This method does only best-effort invalidation and is susceptible to a race condition. If the caller changed the state that could be cached (perhaps files on the storage) and calls this method, there is no guarantee that the cache will not contain stale entries some time after this method returns. This is a similar problem as the one described at google/guava#1881. `ContentCache` doesn't use a Guava Cache, it uses Caffeine. Caffeine offers partial solution to this issue, but not for `invalidateAll` call. To avoid accidental incorrect use of ContentCache, deprecate the `invalidateAll` method, which can be deceptive for the caller and remove it later. * Document ContentCache.invalidate blocking nature * empty
This method does only best-effort invalidation and is susceptible to a
race condition. If the caller changed the state that could be cached
(perhaps files on the storage) and calls this method, there is no
guarantee that the cache will not contain stale entries some time after
this method returns. This is a similar problem as the one described at
google/guava#1881.
ContentCachedoesn't usea Guava Cache, it uses Caffeine. Caffeine offers partial solution to
this issue, but not for
invalidateAllcall. To avoid accidentalincorrect use of ContentCache, deprecate the
invalidateAllmethod,which can be deceptive for the caller and remove it later.