smengcl commented on PR #4567: URL: https://github.com/apache/ozone/pull/4567#issuecomment-1509213712
Thanks @GeorgeJahad for the comment. > I tend to prefer approach 1. > > The guava doc has specific warnings about using weak/soft values: [guava.dev/releases/19.0/api/docs/com/google/common/cache/CacheBuilder.html#weakValues()](https://guava.dev/releases/19.0/api/docs/com/google/common/cache/CacheBuilder.html#weakValues()) > > "Weak values will be garbage collected once they are weakly reachable. This makes them a poor candidate for caching; consider softValues() instead. " > > "Warning: in most circumstances it is better to set a per-cache maximum size instead of using soft references. You should only use this method if you are well familiar with the practical consequences of soft references. " > > So I am hesitant about approach 2. Yes I read the doc before I went through the implementation. Though I should have used `softValues()` in approach 2. Also just found an unbounded cache usage in Hive code base here: https://github.com/apache/hive/blob/master/serde/src/java/org/apache/hadoop/hive/serde2/objectinspector/ObjectInspectorFactory.java#L76-L80 > For me the main problem with approach 1 is that the implementation doesn't yet support try-with-resources. > > Your concern about encapsulation is valid: As you say, "putting the decrement ref count logic in OmSnapshot#close (so that we can wrap OmSnapshot inside a try-with-resources) would also break the encapsulation thus considered hacky by myself." > > Would it be better, if instead of the refCount interface, we had a cacheEntry wrapper class, that manages the reference counting and also implements the close? The cache entry constructor would take an omSnapshot as a paramater, and would have a getter method to all the enduser to access the actual omSnapshot methods? > > So users would always do a try-with-resources{} to get a cache entry and then unwrap that to get the omSnapshot? Or maybe we could make the cache entry implement the IOMetadataReader interface as well, so there would usually be no unwrapping needed. That could work. I had the same idea briefly but I am not too fond of the extra layer either. However, this would preserve the encapsulation. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
