Hi Matthias, I like the alternatives you've listed. One more that might help is if, instead of overloading flush(), we overloaded commitTransaction() to something like commitTransaction(boolean tolerateRecordErrors). This seems slightly cleaner in that it takes the behavioral change we want, which only applies to transactional producers, to an API method that is only used for transactional producers. It would also avoid the issue of whether or not flush() (or a new variant of it with altered semantics) should throw or not. Thoughts?
Hi Alieh, Thanks for the KIP, I like this direction a lot more than the pluggable handler! I share Artem's concerns that enabling this behavior via configuration doesn't seem like a great fit. It's likely that application code will be written in a style that only works with one type of behavior from transactional producers, so requiring that application code to declare its expectations for the behavior of its producer seems more appropriate than, e.g., allowing users deploying that application to tweak a configuration file that gets fed to producers spun up inside it. Cheers, Chris On Wed, Jun 19, 2024 at 10:32 PM Matthias J. Sax <mj...@apache.org> wrote: > Thanks for the KIP Alieh. I actually like the KIP as-is, but think > Arthem raises very good points... > > Seems we have four options on how to move forward? > > 1. add config to allow "silent error clearance" as the KIP proposes > 2. change flush() to clear error and let it throw > 3. add new flushAndThrow()` (or better name) which clears error and > throws > 4. add `flush(boolean clearAndThrow)` and let user pick (and deprecate > existing `flush()`) > > For (2), given that it would be a behavior change, we might also need a > public "feature flag" config. > > It seems, both (1) and (2) have the issue Artem mentioned. (3) and (4) > would be safer to this end, however, for both we kinda get an ugly API? > > Not sure right now if I have any preference. Seems we need to pick some > evil and that there is no clear best solution? Would be good to her from > others what they think > > > -Matthias > > > On 6/18/24 8:39 PM, Artem Livshits wrote: > > Hi Alieh, > > > > Thank you for the KIP. I have a couple of suggestions: > > > > AL1. We should throw an error from flush after we clear it. This would > > make it so that both "send + commit" and "send + flush + commit" (the > > latter looks like just a more verbose way to express the former, and it > > would be intuitive if it behaves the same) would throw if the transaction > > has an error (so if the code is written either way it's going be > correct). > > At the same time, the latter could be extended by the caller to intercept > > exceptions from flush, ignore as needed, and commit the transaction. > This > > solution would keep basic things simple (if someone has code that doesn't > > require advanced error handling, then basic "send + flush + commit" would > > do the right thing) and advanced things possible, an application can add > > try + catch around flush and ignore some errors. > > > > AL2. I'm not sure if config is the best way to express the modification > of > > the "flush" semantics -- the application logic that calls "flush" needs > to > > match the "flush" semantics and configuring semantics in a detached place > > creates a room for bugs due to discrepancies. This can be especially bad > > if the producer loads configuration from a file at run time, in that > case a > > mistake in configuration could break the application because it was > written > > to expect one "flush" semantics but the semantics is switched. Given > that > > the "flush" semantics needs to match the caller's expectation, a way to > > accomplish that would be to pass the caller's expectation to the "flush" > > call by either have a method with a different name or have an overload > with > > a Boolen flag that would configure the semantics (the current method > could > > just redirect to the new one). > > > > -Artem > > > > On Mon, Jun 17, 2024 at 9:09 AM Alieh Saeedi > <asae...@confluent.io.invalid> > > wrote: > > > >> Hi all, > >> > >> I'd like to kick off a discussion for KIP-1059 that suggests adding a > new > >> feature to the Producer flush() method. > >> > >> > >> > https://cwiki.apache.org/confluence/display/KAFKA/KIP-1059%3A+Enable+the+Producer+flush%28%29+method+to+clear+the+latest+send%28%29+error > >> > >> Cheers, > >> Alieh > >> > > >
