zhangshenghang commented on PR #10306: URL: https://github.com/apache/seatunnel/pull/10306#issuecomment-3743157269
<!-- code-pr-reviewer --> Thanks for the detailed review. Here are the key concerns: ## Blocking Issues **1. Concurrency Safety - JdbcSinkWriter.pendingRows** `pendingRows` (ArrayList) is not thread-safe but can be accessed concurrently by `MultiTableWriterRunnable`. The instance-level `batchLock` cannot prevent concurrent access from multiple threads to the same writer. May cause `ConcurrentModificationException`, data corruption, or lost error rows. **Suggested fix**: Use `ConcurrentLinkedQueue` for `pendingRows`, or ensure serialized access at the MultiTableSink layer. **2. ClassLoader Leak Risk - DefaultErrorSinkWorker.drainLoop()** The `contextClassLoader` is not guaranteed during `sinkWriter.write()` execution. If the thread is reused or the classloader is modified elsewhere, SPI loading may fail. **Suggested fix**: Explicitly set `contextClassLoader` inside `drainLoop()` before each write. **3. Data Loss - JdbcSinkWriter.clearPendingRowsIfAutoFlushed()** Clearing `pendingRows` when `autoCommit=true` based on batch size means error rows may never reach the error sink if `swapPendingRowsLocked()` returns an empty list. **Suggested fix**: Only clear `pendingRows` after successful flush/prepare_commit. ## Non-Blocking - Consider documenting behavior for plugins not implementing `SupportRowLevelError` - Add guard for `max_error_ratio_min_records=0` to prevent division by zero - Consider exposing error metrics via `MetricsContext` for monitoring ## Tests LGTM on the suggested test coverage. Adding the MultiTableSink concurrent thread-safety test and the E2E autoCommit + batchSize scenario would be valuable additions. -- 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]
