Hi, Séamus, Thanks for the KIP. We definitely want the input semantic for the producer callback to be consistent. It's probably better to make the input semantic consistent between the producer callback and the interceptor.
If producer.send() throws an ApiException, we may not know the partition for the record. In that case, we probably just want to set the partition to -1 as the interceptor does. Jun On Thu, Nov 11, 2021 at 11:51 AM John Roesler <vvcep...@apache.org> wrote: > Thanks for the reply, Séamus, > > Ah, I missed that the actual value of the placeholder is > that otherwise, you wouldn't know the topic/partition of the > error. > > I guess, on balance, it doesn't feel like this situation > really justifies moving to a new callback interface (to pass > back the topic/partition separately from the other > metadata), even though that might have been nicer in a > vacuum. So, if you think it's nice to have those metadata, > then I think the fact that it's been the de-facto behavior > for many callback situations for a while now, and the fact > that it's the established pattern of the interceptor > indicates that it's probably fine to do as you propose and > just standardize on the placeholder in error cases. > > Thanks! > -John > > On Thu, 2021-11-11 at 17:08 +0000, Séamus Ó Ceanainn wrote: > > 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 <vvcep...@apache.org> 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. > > > > > >
