vbhanuchander-lang commented on PR #15913:
URL: https://github.com/apache/iceberg/pull/15913#issuecomment-5527925621
@UrsSchoenenbergerNu this was closed by the stale bot on 23 May, not on
merit, and #15846 is still
open. @fmorillo7694 independently reproduced it at AWS three days ago. I
verified your fix against
current `main` and found one gap that matters for the 1.12 cut — posting
both so a committer has
less to redo.
**Verified against `main` (`0107f19a`), first-hand:**
- the patch applies cleanly, no conflicts
- with it, `TestIcebergFilesCommitter` is **114 tests, 0 failures**
- reverting *only* `IcebergFilesCommitter.java` and keeping your tests gives
**18 failures**, in
exactly `testPostBarrierDataSurvivesFailedCheckpoint`,
`testPostBarrierDataForFailedCheckpointSurvivesRecovery` and
`testPostBarrierDataMergedWithEarlyDataOnRecovery`
18 is the same count @fmorillo7694 reported, which is a good sign the two
reproductions are the same
thing rather than two different ones.
**The gap: the patch only covers `flink/v2.1`.** `main` now has **v1.20,
v2.1, v2.2 and v2.3**, and
the vulnerable class is in all four — all five references to
`writeResultsSinceLastSnapshot` are
present in each. `IcebergFilesCommitter.java` is **byte-identical across
v2.1, v2.2 and v2.3**
(same sha), and differs only in v1.20. I copied your patched file to v2.2
and v2.3 unchanged and
`:iceberg-flink:iceberg-flink-2.3:compileJava` is green, so for those two it
is a verbatim port;
v1.20 needs the change adapted by hand.
That matters because @fmorillo7694's point is that **`flink/v2.3` is what
would ship the bug in
1.12** — and as it stands this PR would fix v2.1 and leave v2.3 broken. The
tests would want porting
across as well, since they currently exist only under v2.1.
**@mxm** — your question from 10 April, whether the V2 `IcebergSink` is
affected, was answered by
@UrsSchoenenbergerNu on 14 April: `IcebergWriteAggregator` takes the
checkpoint id from a different
place, so V2 looks unaffected. Would you be willing to reopen this? Given a
data-loss bug with two
independent reproductions and a release cut approaching, it seems worth not
restarting from scratch.
@UrsSchoenenbergerNu it is your fix and your analysis, and it should stay
yours. If you would rather
not pick it back up, I am happy to do the mechanical part — port it across
the four version dirs and
the tests with it, under your authorship — just say so. And if I have
misread the version matrix,
please correct me.
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
