aIbrahiim commented on PR #39075: URL: https://github.com/apache/beam/pull/39075#issuecomment-5096177009
> In general, I don't quite think this idea will work. The main reason for needing finalize id's is that Windmill can return Commit status OK before the actual results are durably persisted in the backend. So if the persistence were to fail (e.g. backend worker gets restarted) sometime after the CommitWork returned OK but before the results were persisted, we would incorrectly run the finalize callback. > > I agree with your analysis of the flakiness though. It looks like the tests keep trying to process the element (claiming a new offset of the restriction) up to 3000 times expecting the finalization callback to happen at least once. What's weird is, I'd expect this to be equally flaky on the (streaming) portable worker too. @acrites Thanks for taking a look, and yeah you’re right i took a look again and yes running the callback on CommitStatus.OK is not safe. I misunderstood that OK can come back before the results are actually durable, so if persistence fails after that we could finalize too early. That’s why the finalize-id round trip exists so this approach isn’t going to fix it Yeah I'm glad that the flakiness analysis still lines up and on the portable side, I think it’s less flaky because finalization goes through the Fn API FinalizeBundle path instead of the Windmill applied_finalize_ids best-effort echo. Also the V2 streaming ValidatesRunner suite excludes UsesBoundedSplittableParDo, so that bounded SDF finalizer test doesn’t even run there -- 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]
