Repository navigation
Conversation
…rrent StreamWriter closure on table creation
|
/gemini review |
There was a problem hiding this comment.
📋 Review Summary
This PR mitigates flakiness in StorageApiSinkRowUpdateIT by configuring DirectOptions.setTargetParallelism(1) in runPipelineAndWait, preventing concurrent DirectRunner bundles from sharing and racing on a closed StreamWriter while the destination table is lazily created on initial write failure.
🔍 General Feedback
- Moving
p.run()into thetryblock activates thecatch (Pipeline.PipelineExecutionException e)handler that swallowsExceptions.StreamWriterClosedException(previously unreachable becauseDirectOptions.isBlockOnRun()defaults totrue). If aStreamWriterClosedExceptioncausesp.run()to abort after exhausting bundle retries, those rows were never written, so swallowing the exception masks the real failure and causes a confusing downstream assertion failure inassertRowsWritten. - Beyond the test workaround, the underlying concurrency issue in
StorageApiWriteUnshardedRecords(STORAGE_API_AT_LEAST_ONCEwithCREATE_IF_NEEDED) occurs becauseFAILED_PRECONDITIONfromStreamWriterClosedExceptionis treated as a fatalhasPersistentErrorscondition beforetryCreateTable.call()/RetryType.RETRY_ALL_OPERATIONScan be reached, and synchronousStreamWriterClosedExceptionthrows inrunOperationbypassRetryManager.
💡 Tip: Comment /gemini review on this pull request to trigger a new Gemini code review.
| p.getOptions().as(DirectOptions.class).setTargetParallelism(1); | ||
| try { | ||
| PipelineResult result = p.run(); | ||
| result.waitUntilFinish(); |
There was a problem hiding this comment.
🟡 Medium: Moving p.run() inside the try block makes the catch (Pipeline.PipelineExecutionException e) block active (since DirectOptions.isBlockOnRun() is true by default, p.run() blocks and throws PipelineExecutionException directly).
However, StreamWriterClosedException is thrown when writeStream.appendRows(...) fails during DestinationState.flush(), and only surfaces as a PipelineExecutionException after the DirectRunner exhausts all 4 bundle retries and aborts the pipeline. If this exception is caught and ignored here, the failed bundle's rows were never written to BigQuery, and the test will immediately proceed to assertRowsWritten and fail with a misleading row-mismatch AssertionError (or 404 table not found) while discarding the actual pipeline failure stack trace.
With setTargetParallelism(1) preventing the concurrent bundle race, consider removing the try/catch block that swallows StreamWriterClosedException so that any unexpected failure surfaces with its full exception cause.
|
/gemini review |
There was a problem hiding this comment.
📋 Review Summary
This PR resolves flakiness in StorageApiSinkRowUpdateIT by configuring DirectOptions.setTargetParallelism(1) in runPipelineAndWait to prevent concurrent DirectRunner bundles from racing on a shared StreamWriter during initial table creation. It also removes the obsolete StreamWriterClosedException catch block so unexpected pipeline errors surface directly.
🔍 General Feedback
- The changes are clean, well-scoped, and address the previous review feedback regarding exception handling.
💡 Tip: Comment /gemini review on this pull request to trigger a new Gemini code review.
|
/gemini review |
There was a problem hiding this comment.
📋 Review Summary
This PR resolves flakiness in StorageApiSinkRowUpdateIT and BigQueryIOIcebergManagedTableIT by setting DirectOptions.setTargetParallelism(1) for pipelines writing with STORAGE_API_AT_LEAST_ONCE and CREATE_IF_NEEDED, preventing concurrent DirectRunner bundles from racing on a shared closed StreamWriter during initial table creation. It also cleans up the obsolete StreamWriterClosedException catch block in StorageApiSinkRowUpdateIT.
🔍 General Feedback
- The changes across both integration tests are clean, focused, and properly address the concurrent
StreamWriterrace condition inDirectRunner.
💡 Tip: Comment /gemini review on this pull request to trigger a new Gemini code review.
Summary
Fixes a flake in
StorageApiSinkRowUpdateIT(PreCommit Java GCP IO Directrun https://github.com/apache/beam/actions/runs/42324970282/job/128258023987):Root Cause
In
StorageApiSinkRowUpdateIT,Create.of(items)writes to a non-existent destination table withCreateDisposition.CREATE_IF_NEEDEDandSTORAGE_API_AT_LEAST_ONCE(_defaultstream). WhenDirectRunnerexecutes multiple bundles concurrently on separate threads, they share the same cachedStreamWriterinStorageApiWriteUnshardedRecords.APPEND_CLIENTS. When the first thread's initialAppendRowsRPC fails withPERMISSION_DENIED ... (or it may not exist)prior to table creation (or before table creation propagates to the Storage Write API),ConnectionWorkermarks the sharedStreamWriterclosed. A concurrent bundle callingappendon that sharedStreamWriterbefore cache invalidation receivesStreamWriterClosedException(which carries status codeFAILED_PRECONDITIONand is treated as a persistent error byStorageApiWriteUnshardedRecords). Additionally,p.run()inrunPipelineAndWaitwas previously outside thetryblock even thoughDirectRunner.run()blocks and throws insidep.run().Changes
p.getOptions().as(DirectOptions.class).setTargetParallelism(1)inStorageApiSinkRowUpdateIT.runPipelineAndWaitso bundles execute sequentially on a single thread without racing on a closedStreamWriterduring initial table creation.PipelineResult result = p.run()inside thetryblock inrunPipelineAndWait.Verification
PreCommit Java GCP IO Directworkflow run on this branch: https://github.com/apache/beam/actions/runs/42332692751