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
