Sounds good to me for returning an object than a Long type.
Guozhang On Fri, Nov 10, 2017 at 9:26 AM, Paolo Patierno <ppatie...@live.com> wrote: > Ismael, > > > yes it makes sense. Taking a look to the other methods in the Admin > Client, there is no use case returning a "simple" type : most of them are > Void or complex result represented by classes. > > In order to support future extensibility I like your idea. > > Let's see what's the others opinions otherwise I'll start to implement in > such way. > > > I have updated the KIP and the PR using "recordsToDelete" parameter as > well. > > > Thanks > > > Paolo Patierno > Senior Software Engineer (IoT) @ Red Hat > Microsoft MVP on Azure & IoT > Microsoft Azure Advisor > > Twitter : @ppatierno<http://twitter.com/ppatierno> > Linkedin : paolopatierno<http://it.linkedin.com/in/paolopatierno> > Blog : DevExperience<http://paolopatierno.wordpress.com/> > > > ________________________________ > From: isma...@gmail.com <isma...@gmail.com> on behalf of Ismael Juma < > ism...@juma.me.uk> > Sent: Friday, November 10, 2017 1:15 PM > To: dev@kafka.apache.org > Subject: Re: [VOTE] KIP-204 : adding records deletion operation to the new > Admin Client API > > Paolo, > > You might return the timestamp if the user did a delete by timestamp for > example. But let's maybe hear what others think before we change the KIP. > > Ismael > > On Fri, Nov 10, 2017 at 12:48 PM, Paolo Patierno <ppatie...@live.com> > wrote: > > > Hi Ismael, > > > > > > 1. yes it sounds better. Agree. > > 2. We are using a wrapper class like RecordsToDelete in order to allow > > future operations that won't be based only on specifying an offset. At > same > > time with this wrapper class and static methods (i.e. beforeOffset) the > API > > is really clear to understand. About moving to use a class for wrapping > the > > lowWatermark and not just a Long, do you think that in the future we > could > > have some other information to bring as part of the delete operation ? or > > just for being clearer in terms of API (so not just with a comment) ? > > > > Thanks, > > Paolo. > > > > > > Paolo Patierno > > Senior Software Engineer (IoT) @ Red Hat > > Microsoft MVP on Azure & IoT > > Microsoft Azure Advisor > > > > Twitter : @ppatierno<http://twitter.com/ppatierno> > > Linkedin : paolopatierno<http://it.linkedin.com/in/paolopatierno> > > Blog : DevExperience<http://paolopatierno.wordpress.com/> > > > > > > ________________________________ > > From: isma...@gmail.com <isma...@gmail.com> on behalf of Ismael Juma < > > ism...@juma.me.uk> > > Sent: Friday, November 10, 2017 12:33 PM > > To: dev@kafka.apache.org > > Subject: Re: [VOTE] KIP-204 : adding records deletion operation to the > new > > Admin Client API > > > > Hi Paolo, > > > > The KIP looks good, I have a couple of comments: > > > > 1. partitionsAndOffsets could perhaps be `recordsToDelete`. > > 2. It seems a bit inconsistent that the argument is `RecordsToDelete`, > but > > the result is just a `Long`. Should the result be `DeleteRecords` or > > something like that? It could then have a field logStartOffset or > > lowWatermark instead of having to document it via a comment only. > > > > Ismael > > > > On Tue, Oct 3, 2017 at 6:51 AM, Paolo Patierno <ppatie...@live.com> > wrote: > > > > > Hi all, > > > > > > I didn't see any further discussion around this KIP, so I'd like to > start > > > the vote for it. > > > > > > Just for reference : https://cwiki.apache.org/confl > > > uence/display/KAFKA/KIP-204+%3A+adding+records+deletion+ > > > operation+to+the+new+Admin+Client+API > > > > > > > > > Thanks, > > > > > > Paolo Patierno > > > Senior Software Engineer (IoT) @ Red Hat > > > Microsoft MVP on Azure & IoT > > > Microsoft Azure Advisor > > > > > > Twitter : @ppatierno<http://twitter.com/ppatierno> > > > Linkedin : paolopatierno<http://it.linkedin.com/in/paolopatierno> > > > Blog : DevExperience<http://paolopatierno.wordpress.com/> > > > > > > -- -- Guozhang
