DanielLeens commented on PR #10306: URL: https://github.com/apache/seatunnel/pull/10306#issuecomment-5237307531
Thanks for the update, and apologies again for the earlier posting mess on this thread. Two things to close out from my last full review of da827df05222: First, the CI gate. I called out Issue 9 in that review because the Build workflow for da827df05222 was still queued three hours after the push, with no signal either way. I just checked the apache-side check-runs for that exact head directly, and it is now completed with a success conclusion (finished a little under three and a half hours after it started). That matches what you reported, so Issue 9 is closed: the current head has a genuine green Build run behind it, not just a status label. Second, SEZ9's Issue 1 on JdbcSinkWriter's public surface. I pulled the file at da827df05222 and checked it the same way I'd check any API-compat claim: the original constructor, the one taking a TablePath, SinkWriter.Context, JdbcDialect, JdbcSinkConfig, TableSchema, TableSchema and Integer, is untouched, same parameter list, same order. The only addition is a second constructor without the Context parameter that simply delegates to the original one with a null context, which is a pure additive convenience overload for callers that don't need row-error wiring. Every other public method I'd expect to still be there is: initMultiTableResourceManager, setMultiTableResourceManager, primaryKey, snapshotState, write, prepareCommit(), close(), timerFlush(). The new prepareCommit(long) and classifyRowError(...) are additive, required by the new SupportRowLevelErrorClassifier interface this PR adds. I don't see a removed or altered signature anywhere in this file, so I agree with your reb uttal on this one, this isn't a real API-compat regression. Net effect: with the CI gate now genuinely green and the one open source-level concern raised since my last full review checked out as a non-issue, I don't have a standing source-level blocker on da827df05222 from my side. This is now sitting at the maintainer-approval gate, same as you flagged. I'll keep an eye on this thread and re-review promptly if anything else comes up or a new commit lands. -- 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]
