Thanks for the detailed analysis, Dennis! I tend to agree that issue [761] is not a blocker for 1.0.
However, I'd prefer not to close [761] because, while it may not pose an immediate problem in runtime behaviour, it did cause quite a bit of misunderstanding and confusion for a few developers working with polaris code. So I believe we need to address it. I'd encourage reviews of Pierre's proposed fix [1339]. If it is acceptable to the community, I think it would be a good resolution for issue [761] as it makes the EntityCache class consistent in itself, without having to consider how it is used by the Resolver. If there are objections to [1339], I believe we can still clarify the situation by refactoring the cache code to avoid any doubt that it is used only within the Resolver calls paths. Perhaps we could rename it to ResolverCache and make it a single-purpose class without a public interface... These are just some ideas, I have not given this option a lot of thought yet as I personally prefer fixing the cache class [1339]. Cheers, Dmitri. [761] https://github.com/apache/polaris/issues/761 [1339] https://github.com/apache/polaris/pull/1339 On Fri, May 2, 2025 at 1:02 AM Dennis Huo <huoi...@gmail.com> wrote: > Hello all, > > Following up from today's community sync where we were discussing 1.0 > blockers, I just wanted to finalize discussion on > https://github.com/apache/polaris/issues/761 to check if everyone's > aligned > on the current state of things. > > My understanding is that the originally reported issues regarding > concurrency problems for the EntityCache as a standalone class and its > internal lack of enforcement of cohesion between the "byId" and "byName" > lookups is not actually a concurrency issue for the overall system because > the design of EntityCache today is simply tightly coupled to Resolver.java. > > In particular, the main way to reason about where the Resolver gets its own > internal guarantees about monotonicity of increasing versions it sees is in > the "bulkValidate" method ( > > https://github.com/apache/polaris/blob/3603fa317ad8313f178c09137965bc0e5c7f256d/polaris-core/src/main/java/org/apache/polaris/core/persistence/resolver/Resolver.java#L545 > ) > where the "loadEntitiesChangeTracking" comes directly from the underlying > persistence store to supply the "minVersions" subsequently requested from > the EntityCache. > > I think we also agreed that using an IndexedCache to provide cohesion > between byId and byName would be nice-to-have but in this situation not a > strict requirement for correctness, and if we do encounter any challenges > modifying the EntityCache to use an IndexedCache, we should probably take > our time on it instead of rushing it. > > We discussed that a followup might be to change make EntityCache > package-private in a separate package with Resolver to better make it clear > that the EntityCache isn't intended to be used independently. > > For now though, are there any remaining concerns about removing the > "1.0-blocker" label from https://github.com/apache/polaris/issues/761 ? > > Cheers, > Dennis >
