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