Hi Tom,
Are you planning to extend this KIP to also include dynamic broker config
update (currently covered under AlterConfigPolicy)?
May be worth sending another note to make progress on this KIP since it has
been around a while and reading through the threads, it looks like there
has been a lot of interest in it.
Thank you,
Rajini
On Wed, Jan 9, 2019 at 11:25 AM Tom Bentley <t.j.bent...@gmail.com> wrote:
> Hi Anna and Mickael,
>
> Anna, did you have any comments about the points I made?
>
> Mickael, we really need the vote to be passed before there's even any work
> to do. With the exception of Ismael, the KIP didn't seem to get the
> attention of any of the other committers.
>
> Kind regards,
>
> Tom
>
> On Thu, 13 Dec 2018 at 18:11, Tom Bentley <t.j.bent...@gmail.com> wrote:
>
> > Hi Anna,
> >
> > Firstly, let me apologise again about having missed your previous emails
> > about this.
> >
> > Thank you for the feedback. You raise some valid points about ambiguity.
> > The problem with pulling the metadata into CreateTopicRequest and
> > AlterTopicRequest is that you lose the benefit of being able to eaily
> write
> > a common policy across creation and alter cases. For example, with the
> > proposed design the policy maker could write code like this (forgive my
> > pseudo-Java)
> >
> > public void validateCreateTopic(requestMetadata, ...) {
> > commonPolicy(requestMetadata.requestedState());
> > }
> >
> > public void validateAlterTopic(requestMetadata, ...) {
> > commonPolicy(requestMetadata.requestedState());
> > }
> >
> > private void commonPolicy(RequestedTopicState requestedState) {
> > // ...
> > }
> >
> > I think that's an important feature of the API because (I think) very
> > often the policy maker is interested in defining the universe of
> prohibited
> > configurations without really caring about whether the request is a
> create
> > or an alter. Having a single RequestedTopicState for both create and
> > alter means they can do that trivially in one place. Having different
> > methods in the two Request classes prevents this and forces the policy
> > maker to pick apart the different requestState objects before calling any
> > common method(s).
> >
> > I think my intention at the time (and it's many months ago now, so I
> might
> > not have remembered fully) was that RequestedTopicState would basically
> > represent what the topic would look like after the requested changes were
> > applied (I accept this isn't how it's Javadoc'd in the KIP), rather than
> > representing the request itself. Thus if the request changed the
> assignment
> > of some of the partitions and the policy maker was interested in
> precisely
> > which partitions would be changed, and how, they would indeed have to
> > compute that for themselves by looking up the current topic state from
> the
> > cluster state and seeing how they differed. Indeed they'd have to do this
> > diff even to figure out that the user was requesting a change to the
> topic
> > assigned (or similarly for topic config, etc). To me this is acceptable
> > because I think most people writing such policies are just interested in
> > defining what is not allowed, so giving them a representation of the
> > proposed topic state which they can readily check against is the most
> > direct API. In this interpretation generatedReplicaAssignment() would
> > just be some extra metadata annotating whether any difference between the
> > current and proposed states was directly from the user, or generated on
> the
> > broker. You're right that it's ambiguous when the request didn't actually
> > change the assignment but I didn't envisage policy makers using it except
> > when the assignments differed anyway. To me it would be acceptable to
> > Javadoc this.
> >
> > Given this interpretation of RequestedTopicState as "what the topic would
> > look like after the requested changes were applied" can you see any other
> > problems with the proposal? Or do you have use cases where the policy
> maker
> > is more interested in what the request is changing?
> >
> > Kind regards,
> >
> > Tom
> >
> > On Fri, 7 Dec 2018 at 08:41, Tom Bentley <t.j.bent...@gmail.com> wrote:
> >
> >> Hi Anna and Mickael,
> >>
> >> Sorry for remaining silent on this for so long. I should have time to
> >> look at this again next week.
> >>
> >> Kind regards,
> >>
> >> Tom
> >>
> >> On Mon, 3 Dec 2018 at 10:11, Mickael Maison <mickael.mai...@gmail.com>
> >> wrote:
> >>
> >>> Hi Tom,
> >>>
> >>> This is a very interesting KIP. If you are not going to continue
> >>> working on it, would it be ok for us to grab it and complete it?
> >>> Thanks
> >>> On Thu, Jun 14, 2018 at 7:06 PM Anna Povzner <a...@confluent.io>
> wrote:
> >>> >
> >>> > Hi Tom,
> >>> >
> >>> > Just wanted to check what you think about the comments I made in my
> >>> last
> >>> > message. I think this KIP is a big improvement to our current policy
> >>> > interfaces, and really hope we can get this KIP in.
> >>> >
> >>> > Thanks,
> >>> > Anna
> >>> >
> >>> > On Thu, May 31, 2018 at 3:29 PM, Anna Povzner <a...@confluent.io>
> >>> wrote:
> >>> >
> >>> > > Hi Tom,
> >>> > >
> >>> > >
> >>> > > Thanks for the KIP. I am aware that the voting thread was started,
> >>> but
> >>> > > wanted to discuss couple of concerns here first.
> >>> > >
> >>> > >
> >>> > > I think the coupling of
> >>> RequestedTopicState#generatedReplicaAssignment()
> >>> > > and TopicState#replicasAssignments() does not work well in case
> >>> where the
> >>> > > request deals only with a subset of partitions (e.g., add
> >>> partitions) or no
> >>> > > assignment at all (alter topic config). In particular:
> >>> > >
> >>> > > 1) Alter topic config use case: There is no replica assignment in
> the
> >>> > > request, and generatedReplicaAssignment() returning either true or
> >>> false
> >>> > > is both misleading. The user can interpret this as assignment being
> >>> > > generated or provided by the user originally (e.g., on topic
> >>> create), while
> >>> > > I don’t think we track such thing.
> >>> > >
> >>> > > 2) On add partitions, we may have manual assignment for new
> >>> partitions.
> >>> > > What I understood from the KIP, generatedReplicaAssignment() will
> >>> return
> >>> > > true or false based on whether new partitions were manually
> assigned
> >>> or
> >>> > > not, while TopicState#replicasAssignments() will return replica
> >>> > > assignments for all partitions. I think it is confusing in a way
> that
> >>> > > assignment of old partitions could be auto-generated but new
> >>> partitions are
> >>> > > manually assigned.
> >>> > >
> >>> > > 3) Generalizing #2, suppose in a future, a user can re-assign
> >>> replicas for
> >>> > > a set of partitions.
> >>> > >
> >>> > >
> >>> > > One way to address this with minimal changes to proposed API is to
> >>> rename
> >>> > > RequestedTopicState#generatedReplicaAssignment() to
> >>> RequestedTopicState#manualReplicaAssignment()
> >>> > > and change the API behavior and description to : “True if the
> client
> >>> > > explicitly provided replica assignments in this request, which
> means
> >>> that
> >>> > > some or all assignments returned by
> TopicState#replicasAssignments()
> >>> are
> >>> > > explicitly requested by the user”. The user then will have to diff
> >>> > > TopicState#replicasAssignments() from clusterState and TopicState#
> >>> > > replicasAssignments() from RequestedTopicState, and assume that
> >>> > > assignments that are different are manually assigned (if
> >>> > > RequestedTopicState#manualReplicaAssignment() returns true). We
> will
> >>> > > need to clearly document this and it still seems awkward.
> >>> > >
> >>> > >
> >>> > > I think a cleaner way is to make RequestedTopicState to provide
> >>> replica
> >>> > > assignments only for partitions that were manually assigned
> replicas
> >>> in the
> >>> > > request that is being validated. Similarly, for alter topic
> >>> validation, it
> >>> > > would be nice to make it more clear for the user what has been
> >>> changed. I
> >>> > > remember that you already raised that point earlier by comparing
> >>> current
> >>> > > proposed API with having separate methods for each specific
> command.
> >>> > > However, I agree that it will make it harder to change the
> interface
> >>> in the
> >>> > > future.
> >>> > >
> >>> > >
> >>> > > Could we explore the option of pushing methods that are currently
> in
> >>> > > TopicState to CreateTopicRequest and AlterTopicRequest? TopicState
> >>> will
> >>> > > still be used for requesting current topic state via ClusterState.
> >>> > >
> >>> > > Something like:
> >>> > >
> >>> > > interface CreateTopicRequest extends AbstractRequestMetadata {
> >>> > >
> >>> > > // requested number of partitions or if manual assignment is
> given,
> >>> > > number of partitions in the assignment
> >>> > >
> >>> > > int numPartitions();
> >>> > >
> >>> > > // requested replication factor, or if manual assignment is
> given,
> >>> > > number of replicas in assignment for partition 0
> >>> > >
> >>> > > short replicationFactor();
> >>> > >
> >>> > > // replica assignment requested by the client, or null if
> >>> assignment is
> >>> > > auto-generated
> >>> > >
> >>> > > map<Integer, List<Integer>> manualReplicaAssignment();
> >>> > >
> >>> > > map<String, String> configs();
> >>> > >
> >>> > > }
> >>> > >
> >>> > >
> >>> > > interface AlterTopicRequest extends AbstractRequestMetadata {
> >>> > >
> >>> > > // updated topic configs, or null if not changed
> >>> > >
> >>> > > map<String, String> updatedConfigs();
> >>> > >
> >>> > > // proposed replica assignment in this request, or null. For
> >>> adding new
> >>> > > partitions request, this is proposed replica assignment for new
> >>> partitions.
> >>> > > For replica re-assignment case, this is proposed new assignment.
> >>> > >
> >>> > > map<Integer, List<Integer>> proposedReplicaAssignment();
> >>> > >
> >>> > > // new number of partitions (due to increase/decrease), or null
> if
> >>> > > number of partitions not changed
> >>> > >
> >>> > > Integer updatedNumPartitions()
> >>> > >
> >>> > > }
> >>> > >
> >>> > >
> >>> > > I did not spend much time on my AlterTopicRequest interface
> >>> proposal, but
> >>> > > the idea is basically to return only the parts which were changed.
> >>> The
> >>> > > advantage of this approach over having separate methods for each
> >>> specific
> >>> > > alter topic request is that it is more flexible for future mixing
> of
> >>> what
> >>> > > can be updated in the topic state.
> >>> > >
> >>> > >
> >>> > > What do you think?
> >>> > >
> >>> > >
> >>> > > Thanks,
> >>> > >
> >>> > > Anna
> >>> > >
> >>> > >
> >>> > > On Mon, Oct 9, 2017 at 1:39 AM, Tom Bentley <t.j.bent...@gmail.com
> >
> >>> wrote:
> >>> > >
> >>> > >> I've added RequestedTopicState, as discussed in my last email.
> >>> > >>
> >>> > >> I've also added a paragraph to the migration plan about old
> clients
> >>> making
> >>> > >> policy-violating delete topics or delete records request.
> >>> > >>
> >>> > >> If no further comments a forthcoming in the next day or two then I
> >>> will
> >>> > >> start a vote.
> >>> > >>
> >>> > >> Thanks,
> >>> > >>
> >>> > >> Tom
> >>> > >>
> >>> > >> On 5 October 2017 at 12:41, Tom Bentley <t.j.bent...@gmail.com>
> >>> wrote:
> >>> > >>
> >>> > >> > I'd like to raise a somewhat subtle point about how the proposed
> >>> API
> >>> > >> > should behave.
> >>> > >> >
> >>> > >> > The current CreateTopicPolicy gets passed either the request
> >>> partition
> >>> > >> > count and replication factor, or the requested assignment. So if
> >>> the
> >>> > >> > request had specified partition count and replication factor,
> the
> >>> policy
> >>> > >> > sees a null replicaAssignments(). Likewise if the request
> >>> specified a
> >>> > >> > replica assignment the policy would get back null from
> >>> numPartitions()
> >>> > >> and
> >>> > >> > replicationFactor().
> >>> > >> >
> >>> > >> > These semantics mean the policy can't reject an assignment that
> >>> happened
> >>> > >> > to be auto-generated (or rather, it's obvious to the policy that
> >>> the
> >>> > >> > assignment is auto generated, because it can't see such
> >>> assignments),
> >>> > >> > though it can reject a request because the assignment was
> >>> > >> auto-generated,
> >>> > >> > or vice versa.
> >>> > >> >
> >>> > >> > Retaining these semantics makes the TopicState less symmetric
> >>> between
> >>> > >> it's
> >>> > >> > use in requestedState() and the current state available from the
> >>> > >> > ClusterState, and also less symmetric between its use for
> >>> createTopic()
> >>> > >> and
> >>> > >> > for alterTopic(). This can make it harder to write a policy. For
> >>> > >> example,
> >>> > >> > if I want the policy "the number of partitions must be < 100",
> if
> >>> the
> >>> > >> > requestedState().numPartitions() can be null I need to cope with
> >>> that
> >>> > >> > and figure it out from inspecting the replicasAssignments(). It
> >>> would
> >>> > >> be
> >>> > >> > much better for the policy writer to just be able to write:
> >>> > >> >
> >>> > >> > if (request.requestedState().numPartitions() >= 100)
> >>> > >> > throw new PolicyViolationException("#partitions must be
> <
> >>> 100")
> >>> > >> >
> >>> > >> > An alternative would be to keep the symmetry (and thus
> >>> > >> TopicState.replicasAssignments()
> >>> > >> > would never return null, and TopicState.numPartitions() and
> >>> > >> > TopicState.replicationFactor() could each be primitives), but
> >>> expose the
> >>> > >> > auto-generatedness of the replicaAssignments() explicitly,
> >>> perhaps by
> >>> > >> using
> >>> > >> > a subtype of TopicState for the return type of requestedState():
> >>> > >> >
> >>> > >> > interface RequestedTopicState extends TopicState {
> >>> > >> > /**
> >>> > >> > * True if the {@link TopicState#replicasAssignments()}
> >>> > >> > * in this request we generated by the broker, false if
> >>> > >> > * they were explicitly requested by the client.
> >>> > >> > */
> >>> > >> > boolean generatedReplicaAssignments();
> >>> > >> > }
> >>> > >> >
> >>> > >> > Thoughts?
> >>> > >> >
> >>> > >> > On 4 October 2017 at 11:06, Tom Bentley <t.j.bent...@gmail.com>
> >>> wrote:
> >>> > >> >
> >>> > >> >> Good point. Then I guess I can do those items too. I would also
> >>> need to
> >>> > >> >> do the same changes for DeleteRecordsRequest and Response.
> >>> > >> >>
> >>> > >> >> On 4 October 2017 at 10:37, Ismael Juma <ism...@juma.me.uk>
> >>> wrote:
> >>> > >> >>
> >>> > >> >>> Those two points are related to policies in the following
> sense:
> >>> > >> >>>
> >>> > >> >>> 1. A policy that can't send errors to clients is much less
> >>> useful
> >>> > >> >>> 2. Testing policies is much easier with `validateOnly`
> >>> > >> >>>
> >>> > >> >>> Ismael
> >>> > >> >>>
> >>> > >> >>> On Wed, Oct 4, 2017 at 9:20 AM, Tom Bentley <
> >>> t.j.bent...@gmail.com>
> >>> > >> >>> wrote:
> >>> > >> >>>
> >>> > >> >>> > Thanks Edoardo,
> >>> > >> >>> >
> >>> > >> >>> > I've added that motivation to the KIP.
> >>> > >> >>> >
> >>> > >> >>> > KIP-201 doesn't address two points raised in KIP-170:
> Adding a
> >>> > >> >>> > validationOnly flag to
> >>> > >> >>> > DeleteTopicRequest and adding an error message to
> >>> > >> DeleteTopicResponse.
> >>> > >> >>> > Since those are not policy-related I think they're best left
> >>> out of
> >>> > >> >>> > KIP-201. I suppose it is up to you and Mickael whether to
> >>> narrow the
> >>> > >> >>> scope
> >>> > >> >>> > of KIP-170 to address those points.
> >>> > >> >>> >
> >>> > >> >>> > Thanks again,
> >>> > >> >>> >
> >>> > >> >>> > Tom
> >>> > >> >>> >
> >>> > >> >>> > On 4 October 2017 at 08:20, Edoardo Comar <
> eco...@uk.ibm.com>
> >>> > >> wrote:
> >>> > >> >>> >
> >>> > >> >>> > > Thanks Tom,
> >>> > >> >>> > > looks got to me and KIP-201 could supersede KIP-170
> >>> > >> >>> > > but could you please add a missing motivation bullet that
> >>> was
> >>> > >> behind
> >>> > >> >>> > > KIP-170:
> >>> > >> >>> > >
> >>> > >> >>> > > introducing ClusterState to allow validation of
> >>> create/alter topic
> >>> > >> >>> > request
> >>> > >> >>> > >
> >>> > >> >>> > > not just against the request metadata but also
> >>> > >> >>> > > against the current amount of resources already used in
> the
> >>> > >> cluster
> >>> > >> >>> (eg
> >>> > >> >>> > > number of partitions).
> >>> > >> >>> > >
> >>> > >> >>> > > thanks
> >>> > >> >>> > > Edo
> >>> > >> >>> > > --------------------------------------------------
> >>> > >> >>> > >
> >>> > >> >>> > > Edoardo Comar
> >>> > >> >>> > >
> >>> > >> >>> > > IBM Message Hub
> >>> > >> >>> > >
> >>> > >> >>> > > IBM UK Ltd, Hursley Park, SO21 2JN
> >>> > >> >>> > >
> >>> > >> >>> > >
> >>> > >> >>> > >
> >>> > >> >>> > > From: Tom Bentley <t.j.bent...@gmail.com>
> >>> > >> >>> > > To: dev@kafka.apache.org
> >>> > >> >>> > > Date: 02/10/2017 15:15
> >>> > >> >>> > > Subject: Re: [DISCUSS] KIP-201: Rationalising
> Policy
> >>> > >> >>> interfaces
> >>> > >> >>> > >
> >>> > >> >>> > >
> >>> > >> >>> > >
> >>> > >> >>> > > Hi All,
> >>> > >> >>> > >
> >>> > >> >>> > > I've updated KIP-201 again so there is now a single policy
> >>> > >> interface
> >>> > >> >>> (and
> >>> > >> >>> > > thus a single key by which to configure it) for topic
> >>> creation,
> >>> > >> >>> > > modification, deletion and record deletion, which each
> have
> >>> their
> >>> > >> own
> >>> > >> >>> > > validation method.
> >>> > >> >>> > >
> >>> > >> >>> > > There are still a few loose ends:
> >>> > >> >>> > >
> >>> > >> >>> > > 1. I currently propose validateAlterTopic(), but it would
> be
> >>> > >> >>> possible to
> >>> > >> >>> > > be
> >>> > >> >>> > > more fine grained about this: validateAlterConfig(),
> >>> > >> >>> validAddPartitions()
> >>> > >> >>> > > and validateReassignPartitions(), for example. Obviously
> >>> this
> >>> > >> >>> results in
> >>> > >> >>> > a
> >>> > >> >>> > > policy method per operation, and makes it more clear what
> >>> is being
> >>> > >> >>> > > changed.
> >>> > >> >>> > > I guess the down side is its more work for implementer,
> and
> >>> > >> >>> potentially
> >>> > >> >>> > > makes it harder to change the interface in the future.
> >>> > >> >>> > >
> >>> > >> >>> > > 2. A couple of TODOs about what the TopicState interface
> >>> should
> >>> > >> >>> return
> >>> > >> >>> > > when
> >>> > >> >>> > > a topic's partitions are being reassigned.
> >>> > >> >>> > >
> >>> > >> >>> > > Your thoughts on these or any other points are welcome.
> >>> > >> >>> > >
> >>> > >> >>> > > Thanks,
> >>> > >> >>> > >
> >>> > >> >>> > > Tom
> >>> > >> >>> > >
> >>> > >> >>> > > On 27 September 2017 at 11:45, Paolo Patierno <
> >>> ppatie...@live.com
> >>> > >> >
> >>> > >> >>> > wrote:
> >>> > >> >>> > >
> >>> > >> >>> > > > Hi Ismael,
> >>> > >> >>> > > >
> >>> > >> >>> > > >
> >>> > >> >>> > > > 1. I don't have a real requirement now but "deleting"
> >>> is an
> >>> > >> >>> > operation
> >>> > >> >>> > > > that could be really dangerous so it's always better
> >>> having a
> >>> > >> way
> >>> > >> >>> for
> >>> > >> >>> > > > having more control on that. I know that we have the
> >>> authorizer
> >>> > >> >>> used
> >>> > >> >>> > for
> >>> > >> >>> > > > that (delete on topic) but fine grained control could be
> >>> better
> >>> > >> >>> (even
> >>> > >> >>> > > > already happens for topic deletion).
> >>> > >> >>> > > > 2. I know about the problem of restarting broker due
> to
> >>> > >> changes
> >>> > >> >>> on
> >>> > >> >>> > > > policies but what do you mean by doing that on the
> >>> clients ?
> >>> > >> >>> > > >
> >>> > >> >>> > > >
> >>> > >> >>> > > > Paolo Patierno
> >>> > >> >>> > > > Senior Software Engineer (IoT) @ Red Hat
> >>> > >> >>> > > > Microsoft MVP on Azure & IoT
> >>> > >> >>> > > > Microsoft Azure Advisor
> >>> > >> >>> > > >
> >>> > >> >>> > > > Twitter : @ppatierno<
> >>> > >> >>> > >
> https://urldefense.proofpoint.com/v2/url?u=http-3A__twitter
> >>> .
> >>> > >> >>> > > com_ppatierno&d=DwIBaQ&c=jf_iaSHvJObTbx-siA1ZOg&r=
> >>> > >> >>> > >
> >>> EzRhmSah4IHsUZVekRUIINhltZK7U0OaeRo7hgW4_tQ&m=h-D-nA7uiy1Z-jta5y-
> >>> > >> >>> > > yh7dKgV77XtsUnJ9Rab1gheY&s=43hzTLEDKw2v5Vh0zwkMTaaKD-
> >>> > >> >>> > HdJD8d_F4-Bsw25-Y&e=
> >>> > >> >>> > > >
> >>> > >> >>> > > > Linkedin : paolopatierno<
> >>> > >> >>> > > https://urldefense.proofpoint.com/v2/url?u=http-3A__it.
> >>> > >> >>> > >
> linkedin.com_in_paolopatierno&d=DwIBaQ&c=jf_iaSHvJObTbx-siA1
> >>> > >> ZOg&r=
> >>> > >> >>> > >
> >>> EzRhmSah4IHsUZVekRUIINhltZK7U0OaeRo7hgW4_tQ&m=h-D-nA7uiy1Z-jta5y-
> >>> > >> >>> > > yh7dKgV77XtsUnJ9Rab1gheY&s=Ig0N7Nwf9EHfTJ2pH3jRM1JIdlzXw6
> >>> > >> >>> > R5Drocu0TMRLk&e=
> >>> > >> >>> > > >
> >>> > >> >>> > > > Blog : DevExperience<
> >>> > >> >>> > > https://urldefense.proofpoint.com/v2/url?u=http-3A__
> >>> > >> >>> > >
> >>> paolopatierno.wordpress.com_&d=DwIBaQ&c=jf_iaSHvJObTbx-siA1ZOg&r=
> >>> > >> >>> > >
> >>> EzRhmSah4IHsUZVekRUIINhltZK7U0OaeRo7hgW4_tQ&m=h-D-nA7uiy1Z-jta5y-
> >>> > >> >>> > > yh7dKgV77XtsUnJ9Rab1gheY&s=Tc9NrTtG2GP7-zRjOHkXHfYI0rncO8_
> >>> > >> >>> > jKpedna692z4&e=
> >>> > >> >>> > > >
> >>> > >> >>> > > >
> >>> > >> >>> > > >
> >>> > >> >>> > > > ________________________________
> >>> > >> >>> > > > From: isma...@gmail.com <isma...@gmail.com> on behalf
> of
> >>> Ismael
> >>> > >> >>> Juma <
> >>> > >> >>> > > > ism...@juma.me.uk>
> >>> > >> >>> > > > Sent: Wednesday, September 27, 2017 10:30 AM
> >>> > >> >>> > > > To: dev@kafka.apache.org
> >>> > >> >>> > > > Subject: Re: [DISCUSS] KIP-201: Rationalising Policy
> >>> interfaces
> >>> > >> >>> > > >
> >>> > >> >>> > > > A couple of questions:
> >>> > >> >>> > > >
> >>> > >> >>> > > > 1. Is this a concrete requirement from a user or is it
> >>> > >> >>> hypothetical?
> >>> > >> >>> > > > 2. You sure you would want to do this in the broker
> >>> instead of
> >>> > >> the
> >>> > >> >>> > > clients?
> >>> > >> >>> > > > It's worth remembering that updating broker policies
> >>> involves a
> >>> > >> >>> rolling
> >>> > >> >>> > > > restart of the cluster, so it's not the right place for
> >>> things
> >>> > >> that
> >>> > >> >>> > > change
> >>> > >> >>> > > > frequently.
> >>> > >> >>> > > >
> >>> > >> >>> > > > Ismael
> >>> > >> >>> > > >
> >>> > >> >>> > > > On Wed, Sep 27, 2017 at 11:26 AM, Paolo Patierno <
> >>> > >> >>> ppatie...@live.com>
> >>> > >> >>> > > > wrote:
> >>> > >> >>> > > >
> >>> > >> >>> > > > > Hi Ismael,
> >>> > >> >>> > > > >
> >>> > >> >>> > > > > regarding motivations for delete records, as I said
> >>> during the
> >>> > >> >>> > > discussion
> >>> > >> >>> > > > > on KIP-204, it gives 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 in 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
> >>> others.
> >>> > >> >>> In
> >>> > >> >>> > > such a
> >>> > >> >>> > > > > policy a user could also check the timestamp related
> to
> >>> the
> >>> > >> >>> offset
> >>> > >> >>> > for
> >>> > >> >>> > > > > allowing or not deletion on time base.
> >>> > >> >>> > > > >
> >>> > >> >>> > > > >
> >>> > >> >>> > > > > Paolo Patierno
> >>> > >> >>> > > > > Senior Software Engineer (IoT) @ Red Hat
> >>> > >> >>> > > > > Microsoft MVP on Azure & IoT
> >>> > >> >>> > > > > Microsoft Azure Advisor
> >>> > >> >>> > > > >
> >>> > >> >>> > > > > Twitter : @ppatierno<
> >>> > >> >>> > >
> https://urldefense.proofpoint.com/v2/url?u=http-3A__twitter
> >>> .
> >>> > >> >>> > > com_ppatierno&d=DwIBaQ&c=jf_iaSHvJObTbx-siA1ZOg&r=
> >>> > >> >>> > >
> >>> EzRhmSah4IHsUZVekRUIINhltZK7U0OaeRo7hgW4_tQ&m=h-D-nA7uiy1Z-jta5y-
> >>> > >> >>> > > yh7dKgV77XtsUnJ9Rab1gheY&s=43hzTLEDKw2v5Vh0zwkMTaaKD-
> >>> > >> >>> > HdJD8d_F4-Bsw25-Y&e=
> >>> > >> >>> > > >
> >>> > >> >>> > > > > Linkedin : paolopatierno<
> >>> > >> >>> > > https://urldefense.proofpoint.com/v2/url?u=http-3A__it.
> >>> > >> >>> > >
> linkedin.com_in_paolopatierno&d=DwIBaQ&c=jf_iaSHvJObTbx-siA1
> >>> > >> ZOg&r=
> >>> > >> >>> > >
> >>> EzRhmSah4IHsUZVekRUIINhltZK7U0OaeRo7hgW4_tQ&m=h-D-nA7uiy1Z-jta5y-
> >>> > >> >>> > > yh7dKgV77XtsUnJ9Rab1gheY&s=Ig0N7Nwf9EHfTJ2pH3jRM1JIdlzXw6
> >>> > >> >>> > R5Drocu0TMRLk&e=
> >>> > >> >>> > > >
> >>> > >> >>> > > > > Blog : DevExperience<
> >>> > >> >>> > > https://urldefense.proofpoint.com/v2/url?u=http-3A__
> >>> > >> >>> > >
> >>> paolopatierno.wordpress.com_&d=DwIBaQ&c=jf_iaSHvJObTbx-siA1ZOg&r=
> >>> > >> >>> > >
> >>> EzRhmSah4IHsUZVekRUIINhltZK7U0OaeRo7hgW4_tQ&m=h-D-nA7uiy1Z-jta5y-
> >>> > >> >>> > > yh7dKgV77XtsUnJ9Rab1gheY&s=Tc9NrTtG2GP7-zRjOHkXHfYI0rncO8_
> >>> > >> >>> > jKpedna692z4&e=
> >>> > >> >>> > > >
> >>> > >> >>> > > > >
> >>> > >> >>> > > > >
> >>> > >> >>> > > > > ________________________________
> >>> > >> >>> > > > > From: isma...@gmail.com <isma...@gmail.com> on behalf
> >>> of
> >>> > >> Ismael
> >>> > >> >>> > Juma <
> >>> > >> >>> > > > > ism...@juma.me.uk>
> >>> > >> >>> > > > > Sent: Wednesday, September 27, 2017 10:18 AM
> >>> > >> >>> > > > > To: dev@kafka.apache.org
> >>> > >> >>> > > > > Subject: Re: [DISCUSS] KIP-201: Rationalising Policy
> >>> > >> interfaces
> >>> > >> >>> > > > >
> >>> > >> >>> > > > > A couple more comments:
> >>> > >> >>> > > > >
> >>> > >> >>> > > > > 1. "If this KIP is accepted for Kafka 1.1.0 this
> >>> removal could
> >>> > >> >>> happen
> >>> > >> >>> > > in
> >>> > >> >>> > > > > Kafka 1.2.0 or a later release." -> we only remove
> code
> >>> in
> >>> > >> major
> >>> > >> >>> > > > releases.
> >>> > >> >>> > > > > So, if it's deprecated in 1.1.0, it would be removed
> in
> >>> 2.0.0.
> >>> > >> >>> > > > >
> >>> > >> >>> > > > > 2. Deleting all messages in a topic is not really the
> >>> same as
> >>> > >> >>> > deleting
> >>> > >> >>> > > a
> >>> > >> >>> > > > > topic. The latter will cause consumers and producers
> to
> >>> error
> >>> > >> out
> >>> > >> >>> > > while
> >>> > >> >>> > > > the
> >>> > >> >>> > > > > former will not. It would be good to motivate the need
> >>> for the
> >>> > >> >>> delete
> >>> > >> >>> > > > > records policy more.
> >>> > >> >>> > > > >
> >>> > >> >>> > > > > Ismael
> >>> > >> >>> > > > >
> >>> > >> >>> > > > > On Wed, Sep 27, 2017 at 11:12 AM, Ismael Juma <
> >>> > >> ism...@juma.me.uk
> >>> > >> >>> >
> >>> > >> >>> > > wrote:
> >>> > >> >>> > > > >
> >>> > >> >>> > > > > > Another quick comment: the KIP states that having
> >>> multiple
> >>> > >> >>> > > interfaces
> >>> > >> >>> > > > > > imply that the logic must be in 2 places. That is
> not
> >>> true
> >>> > >> >>> because
> >>> > >> >>> > > the
> >>> > >> >>> > > > > same
> >>> > >> >>> > > > > > class can implement multiple interfaces (this aspect
> >>> was
> >>> > >> >>> considered
> >>> > >> >>> > > > when
> >>> > >> >>> > > > > we
> >>> > >> >>> > > > > > decided to introduce policies incrementally).
> >>> > >> >>> > > > > >
> >>> > >> >>> > > > > > The main reason why I think the original approach
> >>> doesn't
> >>> > >> work
> >>> > >> >>> well
> >>> > >> >>> > > is
> >>> > >> >>> > > > > > that there is no direct mapping between an operation
> >>> and the
> >>> > >> >>> > policy.
> >>> > >> >>> > > > That
> >>> > >> >>> > > > > > is, we initially thought we would have
> >>> create/alter/delete
> >>> > >> >>> topics,
> >>> > >> >>> > > but
> >>> > >> >>> > > > > that
> >>> > >> >>> > > > > > didn't work out as the alter case is better served
> by
> >>> > >> multiple
> >>> > >> >>> > > request
> >>> > >> >>> > > > > > types. Given that, it's a bit awkward to maintain
> the
> >>> > >> original
> >>> > >> >>> > > approach
> >>> > >> >>> > > > > and
> >>> > >> >>> > > > > > a policy for topic management seemed easier to
> >>> understand.
> >>> > >> On
> >>> > >> >>> that
> >>> > >> >>> > > > note,
> >>> > >> >>> > > > > > would `TopicManagementPolicy` be a better name?
> >>> > >> >>> > > > > >
> >>> > >> >>> > > > > > Looking at the updated KIP, I notice that we
> actually
> >>> have a
> >>> > >> >>> > > > > > TopicDeletionPolicy with a separate config. That
> >>> seems to
> >>> > >> be a
> >>> > >> >>> > > halfway
> >>> > >> >>> > > > > > house. Not sure about that.
> >>> > >> >>> > > > > >
> >>> > >> >>> > > > > > Ismael
> >>> > >> >>> > > > > >
> >>> > >> >>> > > > > > On Wed, Sep 27, 2017 at 10:47 AM, Tom Bentley
> >>> > >> >>> > > <t.j.bent...@gmail.com>
> >>> > >> >>> > > > > > wrote:
> >>> > >> >>> > > > > >
> >>> > >> >>> > > > > >> I have updated the KIP to add a common policy
> >>> interface for
> >>> > >> >>> topic
> >>> > >> >>> > > and
> >>> > >> >>> > > > > >> message deletion. This included pulling
> ClusterState
> >>> and
> >>> > >> >>> > TopicState
> >>> > >> >>> > > > > >> interfaces up to the top level so that they can be
> >>> shared
> >>> > >> >>> between
> >>> > >> >>> > > the
> >>> > >> >>> > > > > two
> >>> > >> >>> > > > > >> policies.
> >>> > >> >>> > > > > >>
> >>> > >> >>> > > > > >> Cheers,
> >>> > >> >>> > > > > >>
> >>> > >> >>> > > > > >> Tom
> >>> > >> >>> > > > > >>
> >>> > >> >>> > > > > >> On 26 September 2017 at 18:09, Edoardo Comar <
> >>> > >> >>> eco...@uk.ibm.com>
> >>> > >> >>> > > > wrote:
> >>> > >> >>> > > > > >>
> >>> > >> >>> > > > > >> > Thanks Tom,
> >>> > >> >>> > > > > >> > In my original KIP-170 I mentioned that the
> method
> >>> > >> >>> > > > > >> >
> >>> > >> >>> > > > > >> > public Map<String, Integer>
> topicsPartitionCount();
> >>> > >> >>> > > > > >> >
> >>> > >> >>> > > > > >> > was just a starting point for a general purpose
> >>> > >> ClusterState
> >>> > >> >>> > > > > >> > as it happened to be exactly the info we needed
> >>> for our
> >>> > >> >>> policy
> >>> > >> >>> > > > > >> > implementation :-)
> >>> > >> >>> > > > > >> > it definitely doesn't feel general purpose
> enough.
> >>> > >> >>> > > > > >> >
> >>> > >> >>> > > > > >> > what about
> >>> > >> >>> > > > > >> >
> >>> > >> >>> > > > > >> > interface ClusterState {
> >>> > >> >>> > > > > >> > public TopicState topicState(String
> >>> topicName);
> >>> > >> >>> > > > > >> > public Set<String> topics();
> >>> > >> >>> > > > > >> > }
> >>> > >> >>> > > > > >> >
> >>> > >> >>> > > > > >> > I think that the implementation of ClusterState
> >>> that the
> >>> > >> >>> server
> >>> > >> >>> > > will
> >>> > >> >>> > > > > >> pass
> >>> > >> >>> > > > > >> > to the policy.validate method
> >>> > >> >>> > > > > >> > would just lazily tap into MetadataCache. No need
> >>> for big
> >>> > >> >>> > upfront
> >>> > >> >>> > > > > >> > allocations.
> >>> > >> >>> > > > > >> >
> >>> > >> >>> > > > > >> > ciao,
> >>> > >> >>> > > > > >> > Edo
> >>> > >> >>> > > > > >> >
> --------------------------------------------------
> >>> > >> >>> > > > > >> >
> >>> > >> >>> > > > > >> > Edoardo Comar
> >>> > >> >>> > > > > >> >
> >>> > >> >>> > > > > >> > IBM Message Hub
> >>> > >> >>> > > > > >> >
> >>> > >> >>> > > > > >> > IBM UK Ltd, Hursley Park, SO21 2JN
> >>> > >> >>> > > > > >> >
> >>> > >> >>> > > > > >> >
> >>> > >> >>> > > > > >> >
> >>> > >> >>> > > > > >> > From: Tom Bentley <t.j.bent...@gmail.com>
> >>> > >> >>> > > > > >> > To: dev@kafka.apache.org
> >>> > >> >>> > > > > >> > Date: 26/09/2017 17:39
> >>> > >> >>> > > > > >> > Subject: Re: [DISCUSS] KIP-201:
> >>> Rationalising
> >>> > >> Policy
> >>> > >> >>> > > > interfaces
> >>> > >> >>> > > > > >> >
> >>> > >> >>> > > > > >> >
> >>> > >> >>> > > > > >> >
> >>> > >> >>> > > > > >> > Hi Edoardo,
> >>> > >> >>> > > > > >> >
> >>> > >> >>> > > > > >> >
> >>> > >> >>> > > > > >> > what about a single method in ClusterState
> >>> > >> >>> > > > > >> > >
> >>> > >> >>> > > > > >> > > interface ClusterState {
> >>> > >> >>> > > > > >> > > public Map<String,TopicState>
> >>> topicsState();
> >>> > >> >>> > > > > >> > >
> >>> > >> >>> > > > > >> > > }
> >>> > >> >>> > > > > >> > >
> >>> > >> >>> > > > > >> > > which could return a read-only snapshot of the
> >>> cluster
> >>> > >> >>> > metadata
> >>> > >> >>> > > ?
> >>> > >> >>> > > > > >> > >
> >>> > >> >>> > > > > >> >
> >>> > >> >>> > > > > >> > Sure that would work too. A concern with that is
> >>> that we
> >>> > >> >>> end up
> >>> > >> >>> > > > > >> allocating
> >>> > >> >>> > > > > >> > a potentially rather large amount for the Map and
> >>> the
> >>> > >> >>> > collections
> >>> > >> >>> > > > > >> present
> >>> > >> >>> > > > > >> > in the TopicStates in order to provide the
> >>> snapshot. The
> >>> > >> >>> caller
> >>> > >> >>> > > > might
> >>> > >> >>> > > > > >> only
> >>> > >> >>> > > > > >> > be interested in one item from the TopicState for
> >>> one
> >>> > >> topic
> >>> > >> >>> in
> >>> > >> >>> > > the
> >>> > >> >>> > > > > map.
> >>> > >> >>> > > > > >> > Accessing this information via methods means the
> >>> caller
> >>> > >> only
> >>> > >> >>> > pays
> >>> > >> >>> > > > for
> >>> > >> >>> > > > > >> what
> >>> > >> >>> > > > > >> > they use.
> >>> > >> >>> > > > > >> >
> >>> > >> >>> > > > > >> > Cheers,
> >>> > >> >>> > > > > >> >
> >>> > >> >>> > > > > >> > Tom
> >>> > >> >>> > > > > >> >
> >>> > >> >>> > > > > >> >
> >>> > >> >>> > > > > >> >
> >>> > >> >>> > > > > >> > Unless stated otherwise above:
> >>> > >> >>> > > > > >> > IBM United Kingdom Limited - Registered in
> England
> >>> and
> >>> > >> Wales
> >>> > >> >>> > with
> >>> > >> >>> > > > > number
> >>> > >> >>> > > > > >> > 741598.
> >>> > >> >>> > > > > >> > Registered office: PO Box 41, North Harbour,
> >>> Portsmouth,
> >>> > >> >>> > > Hampshire
> >>> > >> >>> > > > PO6
> >>> > >> >>> > > > > >> 3AU
> >>> > >> >>> > > > > >> >
> >>> > >> >>> > > > > >>
> >>> > >> >>> > > > > >
> >>> > >> >>> > > > > >
> >>> > >> >>> > > > >
> >>> > >> >>> > > >
> >>> > >> >>> > >
> >>> > >> >>> > >
> >>> > >> >>> > >
> >>> > >> >>> > > Unless stated otherwise above:
> >>> > >> >>> > > IBM United Kingdom Limited - Registered in England and
> >>> Wales with
> >>> > >> >>> number
> >>> > >> >>> > > 741598.
> >>> > >> >>> > > Registered office: PO Box 41, North Harbour, Portsmouth,
> >>> Hampshire
> >>> > >> >>> PO6
> >>> > >> >>> > 3AU
> >>> > >> >>> > >
> >>> > >> >>> >
> >>> > >> >>>
> >>> > >> >>
> >>> > >> >>
> >>> > >> >
> >>> > >>
> >>> > >
> >>> > >
> >>>
> >>
>
