At the risk of muddying the waters further, have you considered "RecordsToDelete" as the name of the class? It's both shorter and more descriptive imho.
Also "deleteBefore()" as the factory method name isn't very future proof if we came to support time-based deletion. Something like "beforeOffset()" would be clearer, imho. Putting these together: RecordsToDelete.beforeOffset() seems much clearer to me than DeleteRecordsTarget.deleteBefore() On 23 October 2017 at 08:45, Paolo Patierno <ppatie...@live.com> wrote: > About the name I just started to have a doubt about DeletetionTarget > because it could be bounded to any deletion operation (i.e. delete topic, > ...) and not just what we want now, so records deletion. > > I have updated the KIP-204 using DeleteRecordsTarget so it's clear that > it's related to the delete records operation and what it means, so the > target for such operation. > > > 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: Paolo Patierno <ppatie...@live.com> > Sent: Monday, October 23, 2017 7:38 AM > To: dev@kafka.apache.org > Subject: Re: [DISCUSS] KIP-204 : adding records deletion operation to the > new Admin Client API > > Hi Colin, > > I was using the long primitive in the code but not updated the KIP yet, > sorry ... now it's updated ! > > At same time I agree on using DeletionTarget ... KIP updated ! > > > Regarding the deleteBefore factory method, it's a pattern already used > witn NewPartitions.increaseTo which I think it's really clear and give us > more possibility to evolve this DeletionTarget class if we'll add different > ways to specify such target not only offset based. > > > 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: Colin McCabe <cmcc...@apache.org> > Sent: Friday, October 20, 2017 8:18 PM > To: dev@kafka.apache.org > Subject: Re: [DISCUSS] KIP-204 : adding records deletion operation to the > new Admin Client API > > > /** Describe records to delete */ > > public class DeleteRecords { > > private Long offset; > > "DeleteRecords" doesn't really explain what the class is, though. How > about "DeletionTarget"? Also, why do we need a Long object rather than > a long primitive? > > > > > /** > > * Delete all the records before the given {@code offset} > > */ > > public static DeleteRecords deleteBefore(Long offset) { ... } > > This seems confusing to me. What's wrong with a regular constructor for > DeletionTarget? > > best, > Colin > > > On Fri, Oct 20, 2017, at 01:28, Paolo Patierno wrote: > > Hi all, > > > > > > I have just updated the KIP with your suggestion. > > > > I'm going to continue implementation and tests with these changes, > > waiting for further discussion. > > > > > > 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: Paolo Patierno <ppatie...@live.com> > > Sent: Thursday, October 19, 2017 1:37 PM > > To: dev@kafka.apache.org > > Subject: Re: [DISCUSS] KIP-204 : adding records deletion operation to the > > new Admin Client API > > > > @Colin Yes you are right I'll update the KIP-204 mentioning the related > > ACL permission DELETE on TOPIC > > > > > > @Dong @Ismael > > > > > > Considering future improvements on this, it makes sense to me using a > > class instead of a Long. > > > > Maybe the name could be just "DeleteRecords" (as "NewPartitions") having > > a deleteBefore(Long) factory method for a simple creation when you need > > to delete before the specified offset. > > > > > > 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: Colin McCabe <cmcc...@apache.org> > > Sent: Wednesday, October 18, 2017 3:58 PM > > To: dev@kafka.apache.org > > Subject: Re: [DISCUSS] KIP-204 : adding records deletion operation to the > > new Admin Client API > > > > Having a class there makes sense to me. It also helps clarify what the > > Long represents (a record offset). > > > > regards, > > Colin > > > > > > On Wed, Oct 18, 2017, at 06:19, Dong Lin wrote: > > > Sure. This makes sense. I agree it is better to replace Long with a new > > > class. > > > > > > On Wed, Oct 18, 2017 at 6:16 AM, Ismael Juma <ism...@juma.me.uk> > wrote: > > > > > > > Hi Dong, > > > > > > > > Yes, I mean replacing the `Long` with a class in the map. The class > would > > > > have static factory methods for the various use cases. To use the > > > > `createPartitions` example, there is a `NewPartitions.increaseTo` > method. > > > > > > > > Not sure why you think it's too complicated. It provides better type > > > > safety, it's more informative and makes it easier to evolve. > Thankfully > > > > Java has lambdas for a while now and mapping a collection from one > type to > > > > another is reasonably simple. > > > > > > > > Your suggestion doesn't work because both methods would have the same > > > > "erased" signature. You can't have two overloaded methods that have > the > > > > same signature apart from generic parameters. Also, we'd end up with > 2 > > > > methods in AdminClient. > > > > > > > > Ismael > > > > > > > > > > > > On Wed, Oct 18, 2017 at 1:42 PM, Dong Lin <lindon...@gmail.com> > wrote: > > > > > > > > > Hey Ismael, > > > > > > > > > > To clarify, I think you are saying that we should replace > > > > > "deleteRecords(Map<TopicPartition, Long> partitionsAndOffsets)" > with > > > > > "deleteRecords(Map<TopicPartition, DeleteRecordsParameter> > > > > > partitionsAndOffsets)", where DeleteRecordsParameter should be > include a > > > > > "Long value", and probably "Boolean isBeforeOrAfter" and "Boolean > > > > > isOffsetOrTime" in the future. > > > > > > > > > > I get the point that we want to only include additional parameter > > > > > in DeleteRecordsOptions. I just feel it is a bit overkill to have > a new > > > > > class DeleteRecordsParameter which will only support offset in the > near > > > > > future. This method signature seems a bit too complicated. > > > > > > > > > > How about we use deleteRecords(Map<TopicPartition, Long> > > > > > partitionsAndOffsets) for now, and add deleteRecords(Map< > TopicPartition, > > > > > DeleteRecordsParameter> partitionsAndOffsets) when we need to > support > > > > core > > > > > parameter in the future? By doing this we can make user's > experience > > > > better > > > > > (i.e. not having to instantiate DeleteRecordsParameter) until it is > > > > > necessary to be more complicated. > > > > > > > > > > Dong > > > > > > > > > > On Wed, Oct 18, 2017 at 4:46 AM, Ismael Juma <ism...@juma.me.uk> > wrote: > > > > > > > > > > > Hi Dong, > > > > > > > > > > > > I think it makes sense to use the parameters to define the > specifics of > > > > > the > > > > > > request. However, we would probably want to replace the `Long` > with a > > > > > class > > > > > > (similar to `createPartitions`) instead of relying on > > > > > > `DeleteRecordsOptions`. The latter is typically used for defining > > > > > > additional options, not for defining the core behaviour. > > > > > > > > > > > > Ismael > > > > > > > > > > > > On Wed, Oct 18, 2017 at 12:27 AM, Dong Lin <lindon...@gmail.com> > > > > wrote: > > > > > > > > > > > > > Hey Colin, > > > > > > > > > > > > > > I have also thought about deleteRecordsBeforeOffset so that we > can > > > > keep > > > > > > the > > > > > > > name consistent with the existing API in the Scala > AdminClient. But > > > > > then > > > > > > I > > > > > > > think it may be better to be able to specify in > DeleteRecordsOptions > > > > > > > whether the deletion is before/after timestamp or offset. By > doing > > > > this > > > > > > we > > > > > > > have one API rather than four API in Java AdminClient going > forward. > > > > > What > > > > > > > do you think? > > > > > > > > > > > > > > Thanks, > > > > > > > Dong > > > > > > > > > > > > > > On Tue, Oct 17, 2017 at 11:35 AM, Colin McCabe < > cmcc...@apache.org> > > > > > > wrote: > > > > > > > > > > > > > > > Hi Paolo, > > > > > > > > > > > > > > > > This is a nice improvement. > > > > > > > > > > > > > > > > I agree that the discussion of creating a DeleteTopicPolicy > can > > > > wait > > > > > > > > until later. Perhaps we can do it in a follow-on KIP. > However, we > > > > > do > > > > > > > > need to specify what ACL permissions are needed to invoke > this API. > > > > > > > > That should be in the JavaDoc comments as well. Based on the > > > > > previous > > > > > > > > discussion, I am assuming that this means DELETE on the TOPIC > > > > > resource? > > > > > > > > Can you add this to the KIP? > > > > > > > > > > > > > > > > Right now you have the signature: > > > > > > > > > DeleteRecordsResult deleteRecords(Map<TopicPartition, > Long> > > > > > > > > partitionsAndOffsets) > > > > > > > > > > > > > > > > Since this function is all about deleting records that come > before > > > > a > > > > > > > > certain offset, how about calling it > deleteRecordsBeforeOffset? > > > > That > > > > > > > > way, if we come up with another way of deleting records in > the > > > > future > > > > > > > > (such as a timestamp or transaction-based way) it will not be > > > > > > confusing. > > > > > > > > > > > > > > > > On Mon, Oct 16, 2017, at 20:50, Becket Qin wrote: > > > > > > > > > Hi Paolo, > > > > > > > > > > > > > > > > > > Thanks for the KIP and sorry for being late on the thread. > I am > > > > > > > wondering > > > > > > > > > what is the KafkaFuture<Long> returned by all() call? > Should it > > > > be > > > > > a > > > > > > > > > Map<TopicPartition, Long> instead? > > > > > > > > > > > > > > > > Good point. > > > > > > > > > > > > > > > > cheers, > > > > > > > > Colin > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > Thanks, > > > > > > > > > > > > > > > > > > Jiangjie (Becket) QIn > > > > > > > > > > > > > > > > > > On Thu, Sep 28, 2017 at 3:48 AM, Paolo Patierno < > > > > > ppatie...@live.com> > > > > > > > > > wrote: > > > > > > > > > > > > > > > > > > > Hi, > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > maybe we want to start without the delete records policy > for > > > > now > > > > > > > > waiting > > > > > > > > > > for a real needs. So I'm removing it from the KIP. > > > > > > > > > > > > > > > > > > > > I hope for more comments on this KIP-204 so that we can > start a > > > > > > vote > > > > > > > on > > > > > > > > > > Monday. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > 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: Paolo Patierno <ppatie...@live.com> > > > > > > > > > > Sent: Thursday, September 28, 2017 5:56 AM > > > > > > > > > > To: dev@kafka.apache.org > > > > > > > > > > Subject: Re: [DISCUSS] KIP-204 : adding records deletion > > > > > operation > > > > > > to > > > > > > > > the > > > > > > > > > > new Admin Client API > > > > > > > > > > > > > > > > > > > > Hi, > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > I have just updated the KIP-204 description with the new > > > > > > > > > > TopicDeletionPolicy suggested by the KIP-201. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > 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: Paolo Patierno <ppatie...@live.com> > > > > > > > > > > Sent: Tuesday, September 26, 2017 4:57 PM > > > > > > > > > > To: dev@kafka.apache.org > > > > > > > > > > Subject: Re: [DISCUSS] KIP-204 : adding records deletion > > > > > operation > > > > > > to > > > > > > > > the > > > > > > > > > > new Admin Client API > > > > > > > > > > > > > > > > > > > > Hi Tom, > > > > > > > > > > > > > > > > > > > > as I said in the KIP-201 discussion I'm ok with having a > unique > > > > > > > > > > DeleteTopicPolicy but then it could be useful having more > > > > > > information > > > > > > > > then > > > > > > > > > > just the topic name; partitions and offset for messages > > > > deletion > > > > > > > could > > > > > > > > be > > > > > > > > > > useful for a fine grained use cases. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > 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: Tom Bentley <t.j.bent...@gmail.com> > > > > > > > > > > Sent: Tuesday, September 26, 2017 4:32 PM > > > > > > > > > > To: dev@kafka.apache.org > > > > > > > > > > Subject: Re: [DISCUSS] KIP-204 : adding records deletion > > > > > operation > > > > > > to > > > > > > > > the > > > > > > > > > > new Admin Client API > > > > > > > > > > > > > > > > > > > > Hi Paolo, > > > > > > > > > > > > > > > > > > > > I guess a RecordDeletionPolicy should work at the > partition > > > > > level, > > > > > > > > whereas > > > > > > > > > > the TopicDeletionPolicy should work at the topic level. > But > > > > then > > > > > we > > > > > > > run > > > > > > > > > > into a similar situation as described in the motivation > for > > > > > > KIP-201, > > > > > > > > where > > > > > > > > > > the administrator might have to write+configure two > policies in > > > > > > order > > > > > > > > to > > > > > > > > > > express their intended rules. For example, it's no good > > > > > preventing > > > > > > > > people > > > > > > > > > > from deleting topics if they can delete all the messages > in > > > > those > > > > > > > > topics, > > > > > > > > > > or vice versa. > > > > > > > > > > > > > > > > > > > > On that reasoning, perhaps there should be a single > policy > > > > > > interface > > > > > > > > > > covering topic deletion and message deletion. > Alternatively, > > > > the > > > > > > > topic > > > > > > > > > > deletion API could also invoke the record deletion policy > > > > (before > > > > > > the > > > > > > > > topic > > > > > > > > > > deletion policy I mean). But the former would be more > > > > consistent > > > > > > with > > > > > > > > > > what's proposed in KIP-201. > > > > > > > > > > > > > > > > > > > > Wdyt? I can add this to KIP-201 if you want. > > > > > > > > > > > > > > > > > > > > Cheers, > > > > > > > > > > > > > > > > > > > > Tom > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > On 26 September 2017 at 17:01, Paolo Patierno < > > > > > ppatie...@live.com> > > > > > > > > wrote: > > > > > > > > > > > > > > > > > > > > > Hi Tom, > > > > > > > > > > > > > > > > > > > > > > I think that we could live with the current authorizer > based > > > > on > > > > > > > > delete > > > > > > > > > > > topic (for both, deleting messages and topic as a > whole) but > > > > > then > > > > > > > the > > > > > > > > > > > RecordsDeletePolicy could be even more fine grained > giving > > > > the > > > > > > > > > > possibility > > > > > > > > > > > to avoid deleting messages for specific partitions > (inside > > > > the > > > > > > > > topic) and > > > > > > > > > > > starting from a specific offset. > > > > > > > > > > > > > > > > > > > > > > I could think on some users solutions where they know > exactly > > > > > > what > > > > > > > > the > > > > > > > > > > > partitions means inside of a specific topic (because > they are > > > > > > > using a > > > > > > > > > > > custom partitioner on the producer side) so they know > what > > > > kind > > > > > > of > > > > > > > > > > messages > > > > > > > > > > > are inside a partition allowing to delete them but not > the > > > > > other. > > > > > > > > > > > > > > > > > > > > > > In such a policy a user could also check the timestamp > > > > related > > > > > to > > > > > > > the > > > > > > > > > > > offset for allowing or not deletion on time base. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > Wdyt ? > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > 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: Tom Bentley <t.j.bent...@gmail.com> > > > > > > > > > > > Sent: Tuesday, September 26, 2017 2:55 PM > > > > > > > > > > > To: dev@kafka.apache.org > > > > > > > > > > > Subject: Re: [DISCUSS] KIP-204 : adding records > deletion > > > > > > operation > > > > > > > > to the > > > > > > > > > > > new Admin Client API > > > > > > > > > > > > > > > > > > > > > > Hi Edoardo and Paolo, > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > On 26 September 2017 at 14:10, Paolo Patierno < > > > > > > ppatie...@live.com> > > > > > > > > > > wrote: > > > > > > > > > > > > > > > > > > > > > > > What could be useful use cases for having a > > > > > > RecordsDeletePolicy ? > > > > > > > > > > Records > > > > > > > > > > > > can't be deleted for a topic name ? Starting from a > > > > specific > > > > > > > > offset ? > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > I can imagine some users wanting to prohibit using > this API > > > > > > > > completely. > > > > > > > > > > > Maybe others divide up the topic namespace according > to some > > > > > > > scheme, > > > > > > > > and > > > > > > > > > > so > > > > > > > > > > > it would be allowed for some topics, but not for > others based > > > > > on > > > > > > > the > > > > > > > > > > name. > > > > > > > > > > > Both these could be done using authz, but would be much > > > > simpler > > > > > > to > > > > > > > > > > express > > > > > > > > > > > using a policy. > > > > > > > > > > > > > > > > > > > > > > Since both deleting messages and deleting topics are > > > > authorized > > > > > > > using > > > > > > > > > > > delete operation on the topic, using policies it would > be > > > > > > possible > > > > > > > to > > > > > > > > > > allow > > > > > > > > > > > deleting messages from a topic, but not deleting the > topic > > > > > > itself. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > On 26 September 2017 at 15:27, Edoardo Comar < > > > > > eco...@uk.ibm.com> > > > > > > > > wrote: > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > Our KIP-170 did indeed suggest a TopicDeletePolicy - > but, > > > > as > > > > > > > said, > > > > > > > > for > > > > > > > > > > > our > > > > > > > > > > > > intent an Authorizer implementation will be usable > instead, > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > I guess authorization in the most general sense > encompass es > > > > > both > > > > > > > the > > > > > > > > > > > ACL-based authorization inherent in Authorizer and the > > > > various > > > > > > > > > > > operation-specific *Policies. But they're not the > same. The > > > > > > > Policies > > > > > > > > are > > > > > > > > > > > about deciding on *what* is requested, and the > Authorizer is > > > > > > about > > > > > > > > > > making a > > > > > > > > > > > decision purely on *who* is making the request. It's > quite > > > > > > > > legitimate to > > > > > > > > > > > want to use both, or just one or the other. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > >
