Repository navigation
Conversation
There was a problem hiding this comment.
📋 Review Summary
This PR fixes a race condition (NullPointerException / concurrent cache modification) in StateFetchingIterators.CachingStateIterable when readLater() (via prefetch()) and read() (via iterator()) are invoked concurrently or out-of-order across threads, and resolves a potential hang if ContinuationToken receives a non-empty error response. The refactoring to make FirstPageAndContinuationToken immutable, synchronize prefetch() alongside Blocks.fromCache(), and validate errors on get() is clean, well-reasoned, and accompanied by comprehensive concurrency and error tests.
🔍 General Feedback
- Making
FirstPageAndContinuationTokenimmutable and unifying the synchronization monitor betweenprefetch()andBlocks.fromCache()effectively eliminates the race condition onfirstPageResponseFutureand ensures cache updates happen atomically. - Adding the
if (!stateResponse.getError().isEmpty())check inFirstPageAndContinuationToken.get()properly mirrors the existing check inLazyBlockingStateFetchingIterator.loadNext()and prevents indefinite blocking in downstream runners. - One minor test cleanup observation is noted below regarding unused executor resources in the error test.
💡 Tip: Comment /gemini review on this pull request to trigger a new Gemini code review.
💬 Inline Findings
sdks/java/harness/src/test/java/org/apache/beam/fn/harness/state/StateFetchingIteratorsTest.java(line 259): 🟢 Low:executorandtasksare instantiated here, andexecutor.shutdown()is called at the end oftestErrorOnFirstPage(), but nothing is ever submitted toexecutor. These unused variables can be removed to keep the test clean.
|
Assigning reviewers: R: @ahmedabu98 for label java. This pull request likely touches a core component ("core" label). Please review with scrutiny. Note: If you would like to opt out of this review, comment Available commands:
The PR bot will only process comments in the main thread (not review comments). |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #40716 +/- ##
=============================================
- Coverage 56.16% 0 -56.17%
=============================================
Files 1124 0 -1124
Lines 178336 0 -178336
Branches 1489 0 -1489
=============================================
- Hits 100170 0 -100170
+ Misses 75645 0 -75645
+ Partials 2521 0 -2521
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
return;aftercache.remove(IterableCacheKey.INSTANCE);in bothremove()andappendHelper(). When only a partial prefix is cached (nextToken != null), mutating operations now properly evict the partial cache entry without re-caching truncated blocks.UncachedStateIterable.DecodingIteratorwithDataStreamDecoder: Replaced the customDecodingIteratorwithDataStreams.DataStreamDecoder, enabling continuous stream decoding acrossByteStringchunk boundaries and supporting multi-chunk elements.PrefetchableIterator.isReady()Contract inCachingStateIterator: Delegated tounderlyingStateFetchingIterator.isReady()when a block is not present in cache (instead of unconditionally returning false), allowing async prefetching from the State API to transitionisReady()to true.hasNext(): ReplacedcheckState(existing instanceof BlocksPrefix)with a fallback branch that safely loads the next block from the State API when the cached entry was replaced byMutatedBlocksorEmptyBlocks.seekToContinuationToken(),prefetchedResponse.cancel(true)is now called before clearing the reference to prevent abandoned RPC responses from leaking bandwidth and buffers.hasNext(): Changednew ArrayList<>(currentBlockIndex + 1)tonew ArrayList<>(existing.getBlocks().size() + 1)to prevent internalArrayListreallocation.The newly added unit tests all demonstrate the mentioned bugs.