Hi Christo, I have updated the KIP keeping with option b mechanics. Thanks for the suggestions Andrew.
Regards, Sushant Mahajan On Thu, 23 Oct 2025, 19:34 Christo Lolov, <[email protected]> wrote: > Hello! > > Thanks for enumerating the options Andrew, this is quite helpful! > > I believe the following ordering is from most explicit to least > explicit, at least in my head - b), a) and then c). > > Unless you have some other reservations i.e. correctness or > performance, I wouldn't mind proceeding with b). > > Best, > Christo > > On Wed, 22 Oct 2025 at 19:33, Andrew Schofield > <[email protected]> wrote: > > > > Hi Christo, > > There is one other instance I can think of and that’s also in ShareFetch > from > > KIP-932. There’s an AcknowledgeType enum and the enum values have > equivalent > > integers in the protocol. In the protocol, there is also a special value > which means > > “gap” to represent an offset which doesn’t have a record any longer, > probably due > > to compaction, and it doesn’t correspond to an AcknowledgeType enum > value. > > It corresponds to a record that the consumer cannot consume. > > > > I think there are various uses of -1 as a special value. That’s > essentially what > > we were doing here, but in an int8. > > > > We could perhaps do one of: > > > > a) Just rely on AcknowledgeType 4 (renew) and mandate that the request > > must specify 0 for MaxWaitMs and MaxRecords to show that it doesn’t want > > to fetch additional records at this point. In this situation, the > ShareAcquireMode > > in the request would simply be ignored. > > > > b) Add another boolean field for Renew. If we do that, we should add it > to > > ShareAcknowledge too. > > > > c) Use 255 as a special value, like the KIP is written today. > > > > Let me know what you think. > > > > Thanks, > > Andrew > > > > > > > On 22 Oct 2025, at 17:30, Christo Lolov <[email protected]> > wrote: > > > > > > Heya, > > > > > > Okay, I see. I have a preference that a new field is defined. Either > > > the two things are similar enough that we can use 2 or they are > > > different enough that a new field should be introduced, but I don't > > > think we have reserved two opposite sides of an integer range to > > > represent different concepts elsewhere in the codebase. If there isn't > > > an example of this elsewhere, I would prefer we don't set a precedent > > > with this. > > > > > > Best, > > > Christo > > > > > > On Wed, 22 Oct 2025 at 15:57, Sushant Mahajan <[email protected]> > wrote: > > >> > > >> Hi Christo, > > >> This specific impl was preferred because the ShareAcquireMode values > > >> introduced in kip-1206 are not applicable to the case where a > ShareFetch > > >> request contains RENEW acks. The 2 values defined in kip-1206 apply to > > >> fetching records in poll where the number of records may or may not > exceed > > >> max.poll.records. > > >> > > >> When ShareFetch contains a RENEW ack, none of the values of share > acquire > > >> mode apply. So defining a new value here is logical and serves the > dual > > >> purpose of signalling the broker to not send back records and return > > >> immediately after renewing and having a valid value for > ShareAcquireMode in > > >> case of ShareFetch renewal. > > >> > > >> Regards, > > >> Sushant Mahajan > > >> > > >> On Wed, 22 Oct 2025, 19:09 Christo Lolov, <[email protected]> > wrote: > > >> > > >>> Heya, > > >>> > > >>> Thanks for the quick reply! > > >>> > > >>> I understand. Is there a reason why you chose not to introduce a new > > >>> field instead? > > >>> > > >>> Best, > > >>> Christo > > >>> > > >>> On Wed, 22 Oct 2025 at 13:11, Sushant Mahajan <[email protected]> > > >>> wrote: > > >>>> > > >>>> Hi Christo, > > >>>> We chose this as this as the new field has nothing to with > acquisition of > > >>>> records and it won't be added to the ShareAcquireMode enum either. > > >>>> > > >>>> 255 was chosen with the intention that if some new modes are added > to the > > >>>> enum later, they their ids would line up properly. > > >>>> > > >>>> Though the value is part of acquiremode field, its purpose is quite > > >>>> different. > > >>>> > > >>>> On Wed, 22 Oct 2025, 17:29 Christo Lolov, <[email protected]> > > >>> wrote: > > >>>> > > >>>>> Hello! > > >>>>> > > >>>>> Thanks for the contribution! > > >>>>> > > >>>>> A small question from my side - what is the reasoning behind using > 255 > > >>>>> rather than 2? > > >>>>> > > >>>>> Best, > > >>>>> Christo > > >>>>> > > >>>>> On Wed, 22 Oct 2025 at 12:16, Lianet M. <[email protected]> > wrote: > > >>>>>> > > >>>>>> Thanks for the updates Sushant. No more comments from me. > > >>>>>> > > >>>>>> Best, > > >>>>>> Lianet > > >>>>>> > > >>>>>> On Tue, Oct 21, 2025, 10:26 a.m. Andrew Schofield < > > >>>>> [email protected]> > > >>>>>> wrote: > > >>>>>> > > >>>>>>> Hi Sushant, > > >>>>>>> Thanks for the updates. The way the consumer and broker > communicate > > >>>>>>> the new behaviour is clearer and easier to validate in the latest > > >>>>> update. > > >>>>>>> > > >>>>>>> No more comments from me. > > >>>>>>> > > >>>>>>> Thanks, > > >>>>>>> Andrew > > >>>>>>> > > >>>>>>>> On 21 Oct 2025, at 11:58, Sushant Mahajan <[email protected] > > > > >>>>> wrote: > > >>>>>>>> > > >>>>>>>> Hi Andrew, > > >>>>>>>> Thanks for the additional insights. Have incorporated those. > > >>>>>>>> > > >>>>>>>> AS5: wholly incorporated (will use ERRORS.UNSUPPORTED_VERSION > > >>> (35)) > > >>>>>>>> > > >>>>>>>> AS6: Have mentioned about ignored/zeroed fields. > > >>>>>>>> > > >>>>>>>> Regards, > > >>>>>>>> Sushant Mahajan > > >>>>>>>> > > >>>>>>>> On Fri, 17 Oct 2025, 00:51 Andrew Schofield, < > > >>>>> [email protected]> > > >>>>>>>> wrote: > > >>>>>>>> > > >>>>>>>>> Hi Sushant, > > >>>>>>>>> Thanks for the updates. > > >>>>>>>>> > > >>>>>>>>> AS5: You mention that a broker which does not support v2 of > > >>>>>>>>> ShareFetch/Acknowledge > > >>>>>>>>> would not be able to support RENEW. You then mention that the > > >>>>> exception > > >>>>>>>>> COULD be UnsupportedVersionException. It would be good if the > > >>> KIP > > >>>>>>>>> specified the exception to be used explicitly. > > >>>>>>>>> > > >>>>>>>>> In practice, this will be very unlikely to occur, assuming that > > >>>>> this KIP > > >>>>>>>>> is delivered in Apache Kafka 4.2 because earlier versions are > > >>> not > > >>>>>>>>> production-ready. As a result, I would not define an exception > > >>>>>>> specifically > > >>>>>>>>> for this case and UnsupportedVersionException with an > > >>> appropriate > > >>>>>>>>> message should suffice. > > >>>>>>>>> > > >>>>>>>>> AS6: In the section on broker-side changes, you say that a > > >>>>> ShareFetch > > >>>>>>>>> which includes RENEW acknowledgements will not return any > > >>> records. > > >>>>>>>>> Also, it will return as soon as the acknowledgements have been > > >>>>>>>>> processed, regardless of the value of MaxWaitMs in the request. > > >>>>>>>>> We want to return the acknowledgement error code promptly > > >>> without > > >>>>>>>>> running down the renewed acquisition lock time. > > >>>>>>>>> > > >>>>>>>>> > > >>>>>>>>> Thanks, > > >>>>>>>>> Andrew > > >>>>>>>>> > > >>>>>>>>>> On 13 Oct 2025, at 09:51, Andrew Schofield < > > >>>>> [email protected]> > > >>>>>>>>> wrote: > > >>>>>>>>>> > > >>>>>>>>>> Hi Sushant, > > >>>>>>>>>> Thanks for the updates. > > >>>>>>>>>> > > >>>>>>>>>> AS4: Adding UnsupportedAcknowledgeTypeException is OK, but > > >>>>>>>>>> I don’t think it’s correct to inherit from > > >>>>>>> InvalidConfigurationException. > > >>>>>>>>>> Because that’s an ApiException, it’s related to a specific > > >>> error > > >>>>> code > > >>>>>>>>>> in the Kafka protocol. I don’t think we want to introduce a > new > > >>>>>>>>>> error code in the Kafka protocol, which would imply that the > > >>> broker > > >>>>>>>>>> is able to return it. > > >>>>>>>>>> > > >>>>>>>>>> Two options given that. First, you could instead extend > > >>>>> KafkaException > > >>>>>>>>>> for your new exception class. Second, you could just use the > > >>>>>>>>>> standard Java IllegalArgumentException if the acknowledge type > > >>> is > > >>>>>>>>>> not supported. > > >>>>>>>>>> > > >>>>>>>>>> > > >>>>>>>>>> Thanks, > > >>>>>>>>>> Andrew > > >>>>>>>>>> > > >>>>>>>>>>> On 9 Oct 2025, at 11:48, Sushant Mahajan < > > >>> [email protected]> > > >>>>>>> wrote: > > >>>>>>>>>>> > > >>>>>>>>>>> Hi Andrew, > > >>>>>>>>>>> Thanks for the suggestions. I have incorporated all of them > > >>> and > > >>>>> broken > > >>>>>>>>> down > > >>>>>>>>>>> proposed changes into two sections for better readability. > > >>>>>>>>>>> > > >>>>>>>>>>> For AS2: The proposal is to buffer renew acked records on the > > >>>>> share > > >>>>>>>>>>> consumer side and give them to the application appended to > > >>>>> subsequent > > >>>>>>>>> poll > > >>>>>>>>>>> results. This will happen until the record is re-delivered by > > >>> the > > >>>>>>> broker > > >>>>>>>>>>> after timeout. > > >>>>>>>>>>> > > >>>>>>>>>>> Regards, > > >>>>>>>>>>> Sushant Mahajan > > >>>>>>>>>>> > > >>>>>>>>>>> > > >>>>>>>>>>> On Thu, 9 Oct 2025, 00:58 Andrew Schofield, < > > >>>>>>>>>>> [email protected]> wrote: > > >>>>>>>>>>> > > >>>>>>>>>>>> Hi Sushant, > > >>>>>>>>>>>> Thanks for the KIP. I tried replying previously but messed > > >>> up so > > >>>>>>> let's > > >>>>>>>>> try > > >>>>>>>>>>>> again. > > >>>>>>>>>>>> > > >>>>>>>>>>>> AS1: In the section of Proposed Changes, the KIP states that > > >>> the > > >>>>>>>>>>>> acknowledge(ConsumerRecord, AcknowledgementType) method > > >>>>>>>>>>>> causes RPCs to be sent. This is incorrect. Only the poll and > > >>>>> commit > > >>>>>>>>>>>> methods do this. > > >>>>>>>>>>>> > > >>>>>>>>>>>> AS2: For a situation in which the application is processing > > >>> for > > >>>>> an > > >>>>>>>>>>>> extended period, I would expect it to continue to call > > >>>>>>>>>>>> ShareConsumer.poll(Duration) repeatedly. This method returns > > >>>>>>>>>>>> a set of records to process. In the case where the > > >>> application > > >>>>>>>>>>>> has renewed its acquisition locks, I would expect the > renewed > > >>>>>>>>>>>> records to be returned from the next call to poll(Duration) > > >>> as a > > >>>>>>>>>>>> way of confirming which records are still in the process of > > >>> being > > >>>>>>>>>>>> delivered. > > >>>>>>>>>>>> > > >>>>>>>>>>>> AS3: We should specify the error handling where the > > >>> application > > >>>>>>>>>>>> tries to renew but the broker does not support v2 of the > > >>> updated > > >>>>>>>>>>>> RPCs. I think ShareConsumer.acknowledge() should throw > > >>>>>>>>>>>> an exception in this case. > > >>>>>>>>>>>> > > >>>>>>>>>>>> Thanks, > > >>>>>>>>>>>> Andrew > > >>>>>>>>>>>> ________________________________________ > > >>>>>>>>>>>> From: Sushant Mahajan <[email protected]> > > >>>>>>>>>>>> Sent: 07 October 2025 11:16 > > >>>>>>>>>>>> To: [email protected] <[email protected]> > > >>>>>>>>>>>> Subject: [DISCUSS] KIP-1222: Acquisition lock timeout > > >>> renewal in > > >>>>>>> share > > >>>>>>>>>>>> consumer explicit mode > > >>>>>>>>>>>> > > >>>>>>>>>>>> I’d like to start the discussion for KIP-1222: Acquisition > > >>> lock > > >>>>>>> timeout > > >>>>>>>>>>>> renewal in share consumer explicit mode. > > >>>>>>>>>>>> > > >>>>>>>>>>>> JIRA: https://issues.apache.org/jira/browse/KAFKA-19742 > > >>>>>>>>>>>> KIP Wiki: > > >>>>>>>>>>>> > > >>>>>>>>>>>> > > >>>>>>>>> > > >>>>>>> > > >>>>> > > >>> > https://cwiki.apache.org/confluence/display/KAFKA/KIP-1222%3A+Acquisition+lock+timeout+renewal+in+share+consumer+explicit+mode > > >>>>>>>>>>>> > > >>>>>>>>>>>> Regards, > > >>>>>>>>>>>> Sushant Mahajan > > >>>>>>>>>>>> > > >>>>>>>>>> > > >>>>>>>>> > > >>>>>>>>> > > >>>>>>> > > >>>>>>> > > >>>>> > > >>> > > >
