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. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > >
