Seems the option to use a config does not get a lot of support.
So we need to go with some form or "overload / new method". I think
Chris' point about not coupling it to `flush()` but rather
`commitTransaction()` is actually a very good one; for non-tx case, the
different flush variants would not make sense.
I also like Lianet's idea to pass in some "options" object, so maybe
`commitTransaction(CommitOptions)` or similar could be a good way
forward? It's much better than a `boolean` parameter, aesthetically, as
we as extendable in the future if necessary.
Given that we would pass in an optional parameter, we might not even
need to deprecate the existing `commitTransaction()` method?
-Matthias
On 6/20/24 9:12 AM, Andrew Schofield wrote:
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
