kafka-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Paolo Patierno <ppatie...@live.com>
Subject Re: [DISCUSS] KIP-204 : adding records deletion operation to the new Admin Client API
Date Fri, 20 Oct 2017 08:28:58 GMT
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 <ppatierno@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 <cmccabe@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 <ismael@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 <lindong28@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 <ismael@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 <lindong28@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 <cmccabe@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 <
> > > ppatierno@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 <ppatierno@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 <ppatierno@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.bentley@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 <
> > > ppatierno@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.bentley@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
<
> > > > ppatierno@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
<
> > > ECOMAR@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.
> > > > > > > > >
> > > > > > > >
> > > > > >
> > > > >
> > > >
> > >
> >

Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message