[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