[GitHub] [accumulo] ctubbsii opened a new issue, #2697: CacheEntry SPI has broken generics

[GitBox <gi...@apache.org>]

ctubbsii opened a new issue, #2697:
URL: https://github.com/apache/accumulo/issues/2697

   The CacheEntry class uses a generic method for getIndex to retrieve a Weighable generic type. However, because the generic is on the method, instead of the class, there's no guarantee of retrieving the same Weighable subclass type for repeated calls to the CacheEntry. This forces the calling class to do unnecessary unchecked casts to ensure the result of this method is consistent with the result of a previous call to the same supplier.
   
   This could be made much better if the generic type was put on the CacheEntry, so all calls to getIndex for a given CacheEntry instance would return the same Weighable type, and expect a Supplier that is of that same type.
   
   The risk here is that this breaks users slightly. With runtime erasure, the generic types wouldn't matter for previously compiled code. It also wouldn't normally matter for newly compiled code... except that the Weighable interface is defined inside the CacheEntry class, and it needs to be moved in order to make the class generic instead of the method. That probably would break somebody. However, even then, it's probably low risk to make breaking changes here to fix this, as I doubt it is common for users to write their own block cache implementations.


-- 
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: notifications-unsubscribe@accumulo.apache.org.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [accumulo] ctubbsii commented on issue #2697: CacheEntry SPI has broken generics

[GitBox <gi...@apache.org>]

ctubbsii commented on issue #2697:
URL: https://github.com/apache/accumulo/issues/2697#issuecomment-1289413337

   I was hoping to fix this for 2.1. However, I can't find a sensible path forward here. I think the problems with these SPI classes for the block cache run deeper. For example, the `getIndex` method takes a supplier of a weighable. But, a weighable is just a supplier of an integer. The expectation seems to be that `getIndex` should return a weighable block index reference, but it doesn't have to... it's not clear from the API, though, what the supplier parameter is supposed to be providing. It doesn't actually have to supply anything that represents a block index... it just has to supply a supplier of integers. Internally we don't notice how bad this is because we only ever use the BlockIndex class with it, which works fine. But, we do a bunch of casting to make it work internally. There's no correctness enforced at the API level, and it is very very confusing.
   
   However, I don't think we can easily change the generics to make any of this more sensible, because if we change the method generics to be class-level generics, then we have to propagate the "Weighable" generics parameter all the way up to the BlockCacheManager... which is very disruptive. And, it's not clear we want to make this "Weighable" so prominent everywhere, because it doesn't necessarily have significance to the implementer of these SPI classes. It probably would have been better if we dropped the generics entirely, and just made "BlockIndex", which is the only Weighable class we have, be public API, and used directly. However, that is also a disruptive change, and may restrict SPI implementers.
   
   In addition to all these problems, trying to preserve semver/backwards compatibility with 2.0 is a huge pain. Even if we relaxed that for an SPI class, there's still the above problems that I'm not sure how to deal with, without collaborating more with somebody who understands the use case it was implemented this way for. The javadoc on `getIndex` method is very vague.
   
   So, I don't really know how to move forward on this, and I'm just going to close this as "Won't Fix". Could revisit in future, but maybe it's good enough as-is, and not really a problem for anybody but me (:smiley_cat:).


-- 
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: notifications-unsubscribe@accumulo.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [accumulo] ctubbsii closed issue #2697: CacheEntry SPI has broken generics

[GitBox <gi...@apache.org>]

ctubbsii closed issue #2697: CacheEntry SPI has broken generics
URL: https://github.com/apache/accumulo/issues/2697


-- 
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: notifications-unsubscribe@accumulo.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [accumulo] cshannon commented on issue #2697: CacheEntry SPI has broken generics

[GitBox <gi...@apache.org>]

cshannon commented on issue #2697:
URL: https://github.com/apache/accumulo/issues/2697#issuecomment-1250150972

   @ctubbsii - As I'm still trying to learn more about the internals of Accumulo I was poking around some of the caching implementation today just to see how block caching works and I noticed this issue. Is this still something you want done? If so I can work on it, but at this point maybe we should target it for the 3.0 cleanup release since 2.1 is being wrapped up? 
   
   I started looking at what this change would require and it would touch a good number of files and since it's basically a clean up of the generics (along with a slight chance for breaking users as you noted) it seems like the 3.0 release would be a good time for this. 


-- 
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: notifications-unsubscribe@accumulo.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org