Hi Mickael,
Sure, that makes sense so I've updated the KIP.
Kind regards,
Tom
On Mon, Aug 12, 2019 at 12:23 PM Mickael Maison <mickael.mai...@gmail.com>
wrote:
> Hi Tom,
>
> Thanks for following up on this KIP. This is a great improvement that
> will make policies more powerful and at the same time easier to
> manage.
>
> I just have one question:
> In AbstractRequestMetadata.principal() javadoc, it says the principal
> will be "null" for non authenticated session. Can't we just have the
> default Principal for the Session instead of null? It's possible to
> have Principals for PLAINTEXT sessions or use the default ANONYMOUS
> Principal.
>
> Thanks
>
>
> On Mon, Aug 12, 2019 at 10:52 AM Tom Bentley <tbent...@redhat.com> wrote:
> >
> > Hi folks,
> >
> > As far as I can see the motivation for KIP-201 is still valid, and as far
> > as I'm aware the changes I made to the KIP back in April addressed the
> > previous comments. Since the issue still needs to be addressed I intend
> to
> > start another vote thread in the near future, but before I do I thought
> I'd
> > check whether anyone has any more comments. So please let me know any
> > feedback for this KIP.
> >
> > Many thanks,
> >
> > Tom
> >
> > On Fri, Apr 12, 2019 at 10:46 AM Tom Bentley <tbent...@redhat.com>
> wrote:
> >
> > > Hi Rajini,
> > >
> > > I've made a number of changes to the KIP.
> > >
> > > 1. I've added RequestedTopicState.requestedConfigs(). This is obviously
> > > unrelated to supporting alter broker, but I think it goes some way to
> > > addressing one of the points Anna made last year.
> > > Anna, wdyt?
> > >
> > > 2. I've added BrokerState, RequestedBrokerState and
> > > BrokerManagementPolicy. These are largely similar to the interfaces for
> > > topic management, but the lifecycle of a BrokerManagementPolicy needs
> to be
> > > different.
> > >
> > > That's because a BrokerManagementPolicy ought to be Configurable with
> the
> > > broker config, but obviously the broker config can change. Because a
> > > cluster-scoped config might be changed via a different broker we need
> to
> > > hook into the Zookeeper change notification on the broker configs to
> > > instantiate a new BrokerManagementPolicy when broker policy changes.
> We'd
> > > need to cope with policy implementation change happening concurrently
> with
> > > policy enforcement.
> > > And technically there's a race here: Sending changes to cluster-scoped
> > > configs to multiple brokers could result in non-deterministic policy
> > > enforcement.
> > >
> > > One way to avoid that would be to require changes to cluster-scoped
> > > configs to be sent to the controller.
> > > This complexity is annoying because it seems likely that many policy
> > > implementations won't _actually_ depend on the broker config.
> > >
> > > Thoughts?
> > >
> > > Kind regards,
> > >
> > > Tom
> > >
> > > On Wed, Apr 10, 2019 at 9:48 AM Rajini Sivaram <
> rajinisiva...@gmail.com>
> > > wrote:
> > >
> > >> Thanks Tom.
> > >>
> > >> Once you have updated the KIP to support broker config updates, it
> may be
> > >> good to start a new vote thread since the other one is quite old and
> > >> perhaps the KIP has changed since then.
> > >>
> > >>
> > >> On Wed, Apr 10, 2019 at 3:58 AM Tom Bentley <tbent...@redhat.com>
> wrote:
> > >>
> > >> > Hi Rajini,
> > >> >
> > >> > I'd be happy to do that. I'll try to get it done in the next few
> days.
> > >> >
> > >> > Although there's been quite a lot of interest this, the vote thread
> > >> never
> > >> > got any binding +1, so it's been stuck in limbo for a long time. It
> > >> would
> > >> > be great to get this moving again.
> > >> >
> > >> > Kind regards,
> > >> >
> > >> > Tom
> > >> >
> > >> > On Tue, Apr 9, 2019 at 3:04 PM Rajini Sivaram <
> rajinisiva...@gmail.com>
> > >> > wrote:
> > >> >
> > >> > > 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
> > >> > > > >>> > >> >>> > >
> > >> > > > >>> > >> >>> >
> > >> > > > >>> > >> >>>
> > >> > > > >>> > >> >>
> > >> > > > >>> > >> >>
> > >> > > > >>> > >> >
> > >> > > > >>> > >>
> > >> > > > >>> > >
> > >> > > > >>> > >
> > >> > > > >>>
> > >> > > > >>
> > >> > > >
> > >> > >
> > >> >
> > >>
> > >
>
