Remove unnecessary class-level synchronized in ManifestFiles

Open findepi opened this issue 1 year ago • 1 comments

It was guarding CONTENT_CACHES access, but this field is a concurrent data structure (Caffeine) and does not need external synchronization.

Jun 20 '24 11:06 findepi

@rizaon am i thinking about this right?

Jun 20 '24 11:06 findepi

Sorry for the late review on this @findepi , I was going through https://github.com/apache/iceberg/pull/10494 and trying to understand more about this path and saw this change as well. This particular change seems very safe to me since Caffeine already is threadsafe with respect to a given key. We don't a lock around manipulating the data structure, multiple threads can perform the dropCache and it'll be fine.

Jul 19 '24 01:07 amogh-jahagirdar

good point, @amogh-jahagirdar thank you @amogh-jahagirdar @Fokko for review and merge!

Jul 19 '24 09:07 findepi