sarutak commented on PR #57706: URL: https://github.com/apache/spark/pull/57706#issuecomment-5163738167
Thanks for the cleanup. This change itself LGTM. One nit on the title and PR description. Could you revisit the motivation? **1. The old code did not return stale or corrupted data:** > Concurrent callers could interleave those writes and pair a context from one file version with another version's mtime. The previous implementation wrote `cachedContext` before `lastMtime`, so when a reader observes the new mtime, the new context is guaranteed to be visible (volatile happens-before). The actual worst case was an unnecessary re-parse (cache miss when it could have been a hit), not returning a stale or incorrect context. **2. This change does not fully resolve the multi-thread scenario either:** If two threads call `load()` concurrently while the file rotates (v1 -> v2), the following can happen with the new code: - Thread A reads file content (v1) - Thread B reads file content (v2), writes `CachedToken(v2, contextV2)` - Thread A finds v1 != cached content, writes `CachedToken(v1, contextV1)` -- cache rolls back So the cache can still temporarily hold stale content under concurrent access. This is fine in practice (the return value of each `load()` call is always correct for what that thread read from the file, and the next poll will restore the latest), but it means the PR doesn't fully "make the cache thread-safe" in the strict sense. The real wins of this PR are: 1. Eliminating the need for write-ordering reasoning entirely (simpler, more maintainable code) 2. Switching from mtime to content comparison (more robust on filesystems with coarse timestamp granularity) I'd suggest adjusting the title to something like "Simplify FileTokenIngestor cache with immutable snapshot and content-based rotation detection" to better reflect what the change actually achieves. -- 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]
