Hi Alieh, Thanks for the KIP. I *really* don’t like adding a config which changes the behaviour of the flush() method. We already have too many configs. But I totally understand the problem that you’re trying to solve and some of the other suggestions in this thread seem neater.
Personally, I would add another method to KafkaProducer. Not an overload on flush() because this is not flush() at all. Using Matthias’s options, I prefer (3). Thanks, Andrew > On 20 Jun 2024, at 15:08, Lianet M. <liane...@gmail.com> wrote: > > Hi all, thanks for the KIP Alieh! > > LM1. Totally agree with Artem's point about the config not being the most > explicit/flexible way to express this capability. Getting then to Matthias > 4 options, what I don't like about 3 and 4 is that it seems they might not > age very well? Aren't we going to be wanting some other twist to the flush > semantics that will have us adding yet another param to it, or another > overloaded method? I truly don't have the context to answer that, but if it > feels like a realistic future maybe adding some kind FlushOptions params to > the flush would be better from an extensibility point of view. It would > only have the clearErrors option available for now but could accept any > other we may need. I find that this would remove the "ugliness" Matthias > pointed out for 3. and 4. > > LM2. No matter how we end up expressing the different semantics for flush, > let's make sure we update the KIP on the flush and commitTransaction java > docs. It currently states that flush "clears the last exception" and > commitTransaction "will NOT throw" if called after flush, but it really all > depends on the config/options/method used. > > LM3. I find it would be helpful to include an example to show the new flow > that we're unblocking (I see this as the great gain here): flush with clear > error option enabled -> catch and do whatever error handling we want -> > commitTransaction successfully > > Thanks! > > Lianet > > On Wed, Jun 19, 2024 at 11:26 PM Chris Egerton <fearthecel...@gmail.com> > wrote: > >> 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 >>>>> >>>> >>> >>
