visit2rahul commented on PR #4498: URL: https://github.com/apache/polaris/pull/4498#issuecomment-4510836016
Thank you @adutra for the careful analysis and the wrapper sketch. On the two concerns: 1. The future-Mutiny-implementation-detail is real, and the wrapper pattern is the right shape for it. 2. The eviction-gap race - you yourself noted it's "OK-ish because we don't have strong delivery guarantees" and "unrealistic". Agreed. Would it work to do the same split that worked on #4487? Ship this PR as the minimal `onComplete` race fix (already approved by @nandorKollar and @flyrain), and file a follow-up issue with your wrapper as the starting point. The wrapper seems to have a few details worth iterating on independently - @nandorKollar already flagged the double-lock tradeoff, and the wrapper also doesn't close the durability gap during `@PreDestroy` (the `flush(events)` subscriber keeps running asynchronously after `shutdown()` returns). Would those be better debated in their own thread? Thoughts? On @flyrain's "is there a way to test it" - the race window is microseconds at the Caffeine eviction boundary, so a deterministic test isn't portable (same rationale as the test we dropped from #4487). Happy to add a code comment at each call site explaining the race, or file a follow-up for an integration-style probabilistic test - WDYT? @adutra @nandorKollar @flyrain please review as your time permits. -- 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]
