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