DanielLeens commented on PR #10306:
URL: https://github.com/apache/seatunnel/pull/10306#issuecomment-4968419777

   Rechecked the same unchanged head `ef67c91132f5` after the CI / merge-gate 
fact changed.
   
   ## Runtime path rechecked
   ```text
   Sink ROUTE path
     -> SinkFlowLifeCycle.processDataRecord(record)
         -> writer.write(row)
             -> ErrorHandlingSinkWriter.write(row)
                 -> delegate.write(row)
                 -> row-level error
                 -> ErrorHandler.onError(ctx, row, ex)
                     -> DefaultErrorSinkWriter.write(ctx, row, ex)
                         -> enqueue error row
                         -> background worker writes the configured error sink
                 -> returns normally
         -> SinkFlowLifeCycle still updates main-sink success metrics
         -> SinkFlowLifeCycle still appends SINK_WRITE_DONE
   
   Timer flush path
     -> FlushSignal
         -> SinkFlowLifeCycle.processSignal(signal)
             -> writerContext.getFlushAction().run()
     -> DefaultErrorSinkWriter creates the ErrorData sink writer with its own 
SimpleWriterContext
     -> the ErrorData sink flush action is not composed into the parent 
writerContext flush action
   
   Checkpoint path
     -> SinkFlowLifeCycle.processCheckpointBarrier(barrier)
         -> writer.prepareCommit(barrierId)
         -> writer.snapshotState(barrierId)
             -> ErrorHandlingSinkWriter.snapshotState(checkpointId)
                 -> delegate.snapshotState(checkpointId)
                 -> ErrorHandler.flush()
                     -> DefaultErrorSinkWriter.flush()
         -> running task state/ack path
   
   JDBC no-primary-key failed batch path
     -> JdbcSinkWriter.handleRowLevelBatchFailure(...)
         -> outputFormat.clearBatchSilently()
             -> jdbcStatementExecutor.clearBatch()
     -> BufferedBatchStatementExecutor does not override clearBatch()
     -> JdbcBatchStatementExecutor.clearBatch() default is no-op
   ```
   
   ## Merge conclusion
   ### Conclusion: still needs fixes before merge
   
   1. Blocking items
   - The source-side blockers from my previous Daniel review still stand on 
this unchanged head.
   
   2. Current gate update
   - The CI / merge-gate fact I referenced earlier has changed. The current 
status is `PENDING gate: Build`: 
https://github.com/apache/seatunnel/runs/87016686346
   
   So my technical conclusion is unchanged on merge readiness: this PR still 
needs the earlier code-side fixes first.
   


-- 
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]

Reply via email to