Re: [VOTE] KIP-799: Align behaviour for producer callbacks with documented behaviour

[Séamus Ó Ceanainn]

Hi Kirk,

Thanks for bringing this to my attention! I hadn't found KAFKA-12841
previously, and so I had created KAFKA-13448 (a duplicate issue) and an
associated PR back in November. I was told to create a KIP as it was a
change in behaviour, and that's the reason for this KIP.

PR #11689 implements this KIP and is already merged, so I guess there's now
no need to continue this vote...

Regards,
Séamus.


On Tue, 8 Feb 2022 at 00:41, Kirk True  wrote:

> Hi Seamus,
>
> Is there a conflict between KIP-799 and KAFKA-12841 <
> https://issues.apache.org/jira/browse/KAFKA-12841>? A related fix for the
> latter issue was just merged a few days ago (
> https://github.com/apache/kafka/pull/11689) and it seems like there's
> some overlap or discrepancy between the two.
>
> Thanks,
> Kirk
>
> On Mon, Feb 7, 2022, at 1:20 PM, Séamus Ó Ceanainn wrote:
> > Hi all,
> >
> > Bumping this thread again.
> >
> > Kind reminder that* this KIP addresses a* *bug in the Kafka producer
> client*,
> > where the implemented behaviour of the producer client is not the same as
> > the documented behaviour in the case of callbacks. This has caused
> > production incidents 'in the wild'.
> >
> > Regards,
> > Séamus.
> >
> > On Tue, 25 Jan 2022 at 11:41, Séamus Ó Ceanainn <
> seamus.oceana...@zalando.ie>
> > wrote:
> >
> > > Hi all,
> > >
> > > Bumping this voting thread, as it still needs one more binding vote to
> > > pass.
> > >
> > > Regards,
> > > Séamus.
> > >
> > > On Mon, 6 Dec 2021 at 12:04, Mickael Maison 
> > > wrote:
> > >
> > >> Thanks for the KIP!
> > >> +1 (binding)
> > >>
> > >> On Sat, Dec 4, 2021 at 3:49 AM Luke Chen  wrote:
> > >> >
> > >> > Hi Séamus,
> > >> >
> > >> > Thanks for the update.
> > >> > Looks better now!
> > >> >
> > >> > Thank you.
> > >> > Luke
> > >> >
> > >> > On Sat, Dec 4, 2021 at 12:57 AM Séamus Ó Ceanainn <
> > >> > seamus.oceana...@zalando.ie> wrote:
> > >> >
> > >> > > Hey Luke,
> > >> > >
> > >> > > Thanks for the feedback. I've updated the relevant section to
> > >> hopefully
> > >> > > make it more clear from the KIP itself what placeholder value
> would be
> > >> > > returned.
> > >> > >
> > >> > > Regards,
> > >> > > Séamus.
> > >> > >
> > >> > > On Tue, 30 Nov 2021 at 09:52, Luke Chen 
> wrote:
> > >> > >
> > >> > > > Hi Séamus,
> > >> > > > Thanks for the KIP!
> > >> > > > We definitely want to keep the producer callback consistent for
> all
> > >> types
> > >> > > > of errors.
> > >> > > >
> > >> > > > Just one comment for the KIP:
> > >> > > > In the "Proposed Changes" section, could you please "explicitly"
> > >> describe
> > >> > > > what placeholder you'll return, in addition to adding a
> hyperlink to
> > >> > > other
> > >> > > > places, to make it clear.
> > >> > > >
> > >> > > > +1 (non-binding)
> > >> > > >
> > >> > > > Thank you.
> > >> > > > Luke
> > >> > > >
> > >> > > > On Tue, Nov 30, 2021 at 1:17 PM John Roesler <
> vvcep...@apache.org>
> > >> > > wrote:
> > >> > > >
> > >> > > > > Thanks, Séamus!
> > >> > > > >
> > >> > > > > I'm +1 (binding).
> > >> > > > >
> > >> > > > > On Mon, 2021-11-29 at 16:14 +, Séamus Ó Ceanainn wrote:
> > >> > > > > > Hi everyone,
> > >> > > > > >
> > >> > > > > > I'd like to start a vote for KIP-799: Align behaviour for
> > >> producer
> > >> > > > > > callbacks with documented behaviour
> > >> > > > > > <
> > >> > > > >
> > >> > > >
> > >> > >
> > >>
> https://cwiki.apache.org/confluence/display/KAFKA/KIP-799%3A+Align+behaviour+for+producer+callbacks+with+documented+behaviour
> > >> > > > > >
> > >> > > > > > .
> > >> > > > > >
> > >> > > > > > The KIP proposes a breaking change in the behaviour of
> producer
> > >> > > client
> > >> > > > > > callbacks. The breaking change would align the behaviour of
> > >> callbacks
> > >> > > > > with
> > >> > > > > > the documented behaviour for the method, and makes it
> > >> consistent with
> > >> > > > > > similar methods for producer client interceptors.
> > >> > > > > >
> > >> > > > > > Regards,
> > >> > > > > > Séamus.
> > >> > > > >
> > >> > > > >
> > >> > > >
> > >> > >
> > >>
> > >
> >
>

Re: [VOTE] KIP-799: Align behaviour for producer callbacks with documented behaviour

[Séamus Ó Ceanainn]

Hi all,

Bumping this thread again.

Kind reminder that* this KIP addresses a* *bug in the Kafka producer client*,
where the implemented behaviour of the producer client is not the same as
the documented behaviour in the case of callbacks. This has caused
production incidents 'in the wild'.

Regards,
Séamus.

On Tue, 25 Jan 2022 at 11:41, Séamus Ó Ceanainn 
wrote:

> Hi all,
>
> Bumping this voting thread, as it still needs one more binding vote to
> pass.
>
> Regards,
> Séamus.
>
> On Mon, 6 Dec 2021 at 12:04, Mickael Maison 
> wrote:
>
>> Thanks for the KIP!
>> +1 (binding)
>>
>> On Sat, Dec 4, 2021 at 3:49 AM Luke Chen  wrote:
>> >
>> > Hi Séamus,
>> >
>> > Thanks for the update.
>> > Looks better now!
>> >
>> > Thank you.
>> > Luke
>> >
>> > On Sat, Dec 4, 2021 at 12:57 AM Séamus Ó Ceanainn <
>> > seamus.oceana...@zalando.ie> wrote:
>> >
>> > > Hey Luke,
>> > >
>> > > Thanks for the feedback. I've updated the relevant section to
>> hopefully
>> > > make it more clear from the KIP itself what placeholder value would be
>> > > returned.
>> > >
>> > > Regards,
>> > > Séamus.
>> > >
>> > > On Tue, 30 Nov 2021 at 09:52, Luke Chen  wrote:
>> > >
>> > > > Hi Séamus,
>> > > > Thanks for the KIP!
>> > > > We definitely want to keep the producer callback consistent for all
>> types
>> > > > of errors.
>> > > >
>> > > > Just one comment for the KIP:
>> > > > In the "Proposed Changes" section, could you please "explicitly"
>> describe
>> > > > what placeholder you'll return, in addition to adding a hyperlink to
>> > > other
>> > > > places, to make it clear.
>> > > >
>> > > > +1 (non-binding)
>> > > >
>> > > > Thank you.
>> > > > Luke
>> > > >
>> > > > On Tue, Nov 30, 2021 at 1:17 PM John Roesler 
>> > > wrote:
>> > > >
>> > > > > Thanks, Séamus!
>> > > > >
>> > > > > I'm +1 (binding).
>> > > > >
>> > > > > On Mon, 2021-11-29 at 16:14 +, Séamus Ó Ceanainn wrote:
>> > > > > > Hi everyone,
>> > > > > >
>> > > > > > I'd like to start a vote for KIP-799: Align behaviour for
>> producer
>> > > > > > callbacks with documented behaviour
>> > > > > > <
>> > > > >
>> > > >
>> > >
>> https://cwiki.apache.org/confluence/display/KAFKA/KIP-799%3A+Align+behaviour+for+producer+callbacks+with+documented+behaviour
>> > > > > >
>> > > > > > .
>> > > > > >
>> > > > > > The KIP proposes a breaking change in the behaviour of producer
>> > > client
>> > > > > > callbacks. The breaking change would align the behaviour of
>> callbacks
>> > > > > with
>> > > > > > the documented behaviour for the method, and makes it
>> consistent with
>> > > > > > similar methods for producer client interceptors.
>> > > > > >
>> > > > > > Regards,
>> > > > > > Séamus.
>> > > > >
>> > > > >
>> > > >
>> > >
>>
>

Re: [VOTE] KIP-799: Align behaviour for producer callbacks with documented behaviour

[Séamus Ó Ceanainn]

Hi all,

Bumping this voting thread, as it still needs one more binding vote to pass.

Regards,
Séamus.

On Mon, 6 Dec 2021 at 12:04, Mickael Maison 
wrote:

> Thanks for the KIP!
> +1 (binding)
>
> On Sat, Dec 4, 2021 at 3:49 AM Luke Chen  wrote:
> >
> > Hi Séamus,
> >
> > Thanks for the update.
> > Looks better now!
> >
> > Thank you.
> > Luke
> >
> > On Sat, Dec 4, 2021 at 12:57 AM Séamus Ó Ceanainn <
> > seamus.oceana...@zalando.ie> wrote:
> >
> > > Hey Luke,
> > >
> > > Thanks for the feedback. I've updated the relevant section to hopefully
> > > make it more clear from the KIP itself what placeholder value would be
> > > returned.
> > >
> > > Regards,
> > > Séamus.
> > >
> > > On Tue, 30 Nov 2021 at 09:52, Luke Chen  wrote:
> > >
> > > > Hi Séamus,
> > > > Thanks for the KIP!
> > > > We definitely want to keep the producer callback consistent for all
> types
> > > > of errors.
> > > >
> > > > Just one comment for the KIP:
> > > > In the "Proposed Changes" section, could you please "explicitly"
> describe
> > > > what placeholder you'll return, in addition to adding a hyperlink to
> > > other
> > > > places, to make it clear.
> > > >
> > > > +1 (non-binding)
> > > >
> > > > Thank you.
> > > > Luke
> > > >
> > > > On Tue, Nov 30, 2021 at 1:17 PM John Roesler 
> > > wrote:
> > > >
> > > > > Thanks, Séamus!
> > > > >
> > > > > I'm +1 (binding).
> > > > >
> > > > > On Mon, 2021-11-29 at 16:14 +, Séamus Ó Ceanainn wrote:
> > > > > > Hi everyone,
> > > > > >
> > > > > > I'd like to start a vote for KIP-799: Align behaviour for
> producer
> > > > > > callbacks with documented behaviour
> > > > > > <
> > > > >
> > > >
> > >
> https://cwiki.apache.org/confluence/display/KAFKA/KIP-799%3A+Align+behaviour+for+producer+callbacks+with+documented+behaviour
> > > > > >
> > > > > > .
> > > > > >
> > > > > > The KIP proposes a breaking change in the behaviour of producer
> > > client
> > > > > > callbacks. The breaking change would align the behaviour of
> callbacks
> > > > > with
> > > > > > the documented behaviour for the method, and makes it consistent
> with
> > > > > > similar methods for producer client interceptors.
> > > > > >
> > > > > > Regards,
> > > > > > Séamus.
> > > > >
> > > > >
> > > >
> > >
>

Re: [VOTE] KIP-799: Align behaviour for producer callbacks with documented behaviour

[Séamus Ó Ceanainn]

Hey Luke,

Thanks for the feedback. I've updated the relevant section to hopefully
make it more clear from the KIP itself what placeholder value would be
returned.

Regards,
Séamus.

On Tue, 30 Nov 2021 at 09:52, Luke Chen  wrote:

> Hi Séamus,
> Thanks for the KIP!
> We definitely want to keep the producer callback consistent for all types
> of errors.
>
> Just one comment for the KIP:
> In the "Proposed Changes" section, could you please "explicitly" describe
> what placeholder you'll return, in addition to adding a hyperlink to other
> places, to make it clear.
>
> +1 (non-binding)
>
> Thank you.
> Luke
>
> On Tue, Nov 30, 2021 at 1:17 PM John Roesler  wrote:
>
> > Thanks, Séamus!
> >
> > I'm +1 (binding).
> >
> > On Mon, 2021-11-29 at 16:14 +, Séamus Ó Ceanainn wrote:
> > > Hi everyone,
> > >
> > > I'd like to start a vote for KIP-799: Align behaviour for producer
> > > callbacks with documented behaviour
> > > <
> >
> https://cwiki.apache.org/confluence/display/KAFKA/KIP-799%3A+Align+behaviour+for+producer+callbacks+with+documented+behaviour
> > >
> > > .
> > >
> > > The KIP proposes a breaking change in the behaviour of producer client
> > > callbacks. The breaking change would align the behaviour of callbacks
> > with
> > > the documented behaviour for the method, and makes it consistent with
> > > similar methods for producer client interceptors.
> > >
> > > Regards,
> > > Séamus.
> >
> >
>

[VOTE] KIP-799: Align behaviour for producer callbacks with documented behaviour

[Séamus Ó Ceanainn]

Hi everyone,

I'd like to start a vote for KIP-799: Align behaviour for producer
callbacks with documented behaviour

.

The KIP proposes a breaking change in the behaviour of producer client
callbacks. The breaking change would align the behaviour of callbacks with
the documented behaviour for the method, and makes it consistent with
similar methods for producer client interceptors.

Regards,
Séamus.

Re: [DISCUSS] KIP-799 Align behaviour for producer callbacks with documented behaviour

[Séamus Ó Ceanainn]

Hey John,

> did you consider just going back to the original behavior?

I hadn't considered going back to the exact original behaviour as I think
there's a valid point made in discussions around KAFKA-7412 (I
forget whether in a JIRA or PR comment) that returning the topic partition
when available can be useful for users. Something I did consider is to
include the topic partition separately to the metadata value when
exceptions occur so that metadata could still be null in those cases while
still having topic partition data available.

My opinion is that this other behaviour would be nicer (where returned
metadata is null but topic partition information is still available),
however it would not be consistent with the implementation of
ProducerInterceptor.onAcknowledgement method. I would tend to favour
consistency in this case (as both methods are handled very similarly in
code), and I don't think there's a strong argument to make a breaking
change to ProducerInterceptor when there is nothing currently broken in
that implementation (like there currently is with Callback).

Of course if the general consensus is that consistency between the
behaviour of the two methods (ProducerInterceptor.onAcknowledgement and
Callback.onCompletion) does not matter, or that a change in the behaviour
of ProducerInterceptor.onAcknowledgement should also be included in the
scope of this KIP, I'm open to updating the KIP to reflect that.

> Although it’s technically an implementation detail (which doesn’t need to
be in a KIP), I like the fact that you’re planning to refactor the code to
enforce consistent handling of the callbacks.

I wasn't entirely sure how to deal with changes to the interfaces within
the 'clients.producer.internals' package, so I thought it was best to err
on the side of including too much in the KIP.  I'll remove the unnecessary
detail to ensure the discussion doesn't get derailed, for anyone interested
in implementation details there is a draft PR linked in the KIP with that
refactoring done, so any discussion on that topic can take place in Github
/ JIRA.

Regards,
Séamus.

On Thu, 11 Nov 2021 at 14:33, John Roesler  wrote:

> Thanks for the KIP, Séamus!
>
> I agree that the current situation you’re describing doesn’t seem ideal,
> and it’s probably worth a slight behavior change to fix it.
>
> It’s too bad that we introduced that placeholder record, since it seems
> less error prone for users if we have the invariant that exactly one
> argument is non-null. I’m concerned (as reported in KAFKA-7412) that people
> could mistake the placeholder for a successful ack. Since we’re considering
> some breakage to fix this inconsistency, did you consider just going back
> to the original behavior?
>
> Although it’s technically an implementation detail (which doesn’t need to
> be in a KIP), I like the fact that you’re planning to refactor the code to
> enforce consistent handling of the callbacks.
>
> Thanks,
> John
>
> On Thu, Nov 11, 2021, at 07:25, Séamus Ó Ceanainn wrote:
> > Hi,
> >
> > As outlined in KIP-799: Align behaviour for producer callbacks with
> > documented behaviour
> > <
> https://cwiki.apache.org/confluence/display/KAFKA/KIP-799%3A+Align+behaviour+for+producer+callbacks+with+documented+behaviour
> >,
> > there is an inconsistency between the documented behaviour and
> > implementation of producer callbacks for the Kafka client. The KIP
> > proposes
> > breaking changes to the implementation of the Kafka producer client to
> > align the implementation with the documented behaviour, and includes a
> > link
> > to a PR containing a tested implementation of the changes being
> > recommended.
> >
> > There is a need to take action here as a breaking change was previously
> > introduced accidentally, and the documentation was later updated to try
> to
> > reflect those breaking changes. I believe the main discussion here is
> > around the most appropriate behaviour for callbacks, which will inform
> > whether the implementation, documentation or a combination of both should
> > be updated. Right now, the documented behaviour aligns closely with the
> > implementation of ProducerInterceptors, and as a result I would favor
> > keeping the documented behaviour and updating the implementation to
> match,
> > as that would provide greater consistency between Callbacks and
> > Interceptors implementations for producers.
> >
> > Regards,
> > Séamus.
>

[DISCUSS] KIP-799 Align behaviour for producer callbacks with documented behaviour

[Séamus Ó Ceanainn]

Hi,

As outlined in KIP-799: Align behaviour for producer callbacks with
documented behaviour
,
there is an inconsistency between the documented behaviour and
implementation of producer callbacks for the Kafka client. The KIP proposes
breaking changes to the implementation of the Kafka producer client to
align the implementation with the documented behaviour, and includes a link
to a PR containing a tested implementation of the changes being recommended.

There is a need to take action here as a breaking change was previously
introduced accidentally, and the documentation was later updated to try to
reflect those breaking changes. I believe the main discussion here is
around the most appropriate behaviour for callbacks, which will inform
whether the implementation, documentation or a combination of both should
be updated. Right now, the documented behaviour aligns closely with the
implementation of ProducerInterceptors, and as a result I would favor
keeping the documented behaviour and updating the implementation to match,
as that would provide greater consistency between Callbacks and
Interceptors implementations for producers.

Regards,
Séamus.