davidzollo commented on PR #12232: URL: https://github.com/apache/seatunnel/pull/12232#issuecomment-5661967712
Thanks for the approval, @davidzollo. No new commit since my last full re-review (head is still `ffb08361ee5`), so this is a status confirmation rather than a fresh pass. To recap where things stood on that head: the two real correctness bugs found across earlier rounds — checkpoint-time data loss in `FirebaseSinkWriter.prepareCommit()` and the silent delete no-op when `support_deletes=true` has no resolvable primary key — were fixed at the root cause and I re-verified both independently against this exact head (the newest commit only touched `FirebaseHttpClient.buildUrl()`, to fix URL construction when `baseUrl` already carries a query string, which doesn't touch the writer/flush path at all). `getWriteCatalogTable()` (@nzw921rx's earlier finding) is also still implemented. Live check just now: CI is fully green (`Build`, `Notify test workflow`, `labeler` all success). `mergeable_state` is `blocked` — the remaining gap looks to be that @nzw921rx's `CHANGES_REQUESTED` from 2026-09-09 (the `getWriteCatalogTable()` point, since resolved) hasn't been re-reviewed/cleared yet, not any open code issue from my side. The only items still open are the two non-blocking follow-ups from my last review (unit coverage for `prepareCommit()`'s flush and the delete-validation failure path; no other blockers) — neither affects mergeability. Thanks again for the review and the welcome note. -- 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]
