Re: [DISCUSS] KIP-201: Rationalising Policy interfaces

[Tom Bentley]

; > >> > > > >>> > > 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> manualReplicaAssignment();
> > >> > > > >>> > >
> > >> > > > >>> > >  map configs();
> > >> > > > >>> > >
> > >> > > > >>> > > }
> > >> > > > >>> > >
> > >> > > > >>> > >
> > >> > > > >>> > > interface AlterTopicRequest extends
> AbstractRequestMetadata
> > >> {
> > >> > > > >>> > >
> > >> > > > >>> > >   // updated topic configs, or null if not changed
> > >> > > > >>> > >
> > >> > > > >>> > >   map 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>
> 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
> > >> > > > >>> > >
> > >> > > 

Re: [DISCUSS] KIP-201: Rationalising Policy interfaces

[Mickael Maison]

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

Re: [DISCUSS] KIP-201: Rationalising Policy interfaces

[Tom Bentley]

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> manualReplicaAssignment();
>> > > > >>> > >
>> > > > >>> > >  map configs();
>> > > > >>> > >
>> > > > >>> > > }
>> > > > >>> > >
>> > > > >>> > >
>> > > > >>> > > interface AlterTopicRequest extends AbstractRequestMetadata
>> {
>> > > > >>> > >
>> > > > >>> > >   // updated topic configs, or null if not changed
>> > > > >>> > >
>> > > > >>> > >   map 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> 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.
>> > > > >>> > >>
>> > > > >>> > >

Re: [DISCUSS] KIP-201: Rationalising Policy interfaces

[Tom Bentley]

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

Re: [DISCUSS] KIP-201: Rationalising Policy interfaces

[Rajini Sivaram]

> > >>> > >  // replica assignment requested by the client, or null if
> > > >>> assignment is
> > > >>> > > auto-generated
> > > >>> > >
> > > >>> > >  map> manualReplicaAssignment();
> > > >>> > >
> > > >>> > >  map configs();
> > > >>> > >
> > > >>> > > }
> > > >>> > >
> > > >>> > >
> > > >>> > > interface AlterTopicRequest extends AbstractRequestMetadata {
> > > >>> > >
> > > >>> > >   // updated topic configs, or null if not changed
> > > >>> > >
> > > >>> > >   map 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> 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
> >

Re: [DISCUSS] KIP-201: Rationalising Policy interfaces

[Tom Bentley]

est 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 
> > >>> 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:
> > >>> > >> >
> &g

Re: [DISCUSS] KIP-201: Rationalising Policy interfaces

[Rajini Sivaram]

 >>> > >> > 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 
> >>> 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 
> >>> wrote:
> >>> > >> >>
> >>> > >> >>> Those two points are related to policies in the follo

Re: [DISCUSS] KIP-201: Rationalising Policy interfaces

[Tom Bentley]

icy "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 
>>> 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 
>>> 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 
>>> > >> 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:
>>> > >> >>> >

Re: [DISCUSS] KIP-201: Rationalising Policy interfaces

[Tom Bentley]

t;> > >
>> > >
>> > > 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 
>> 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 
>> 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 r

Re: [DISCUSS] KIP-201: Rationalising Policy interfaces

[Tom Bentley]

Assignments()
> > >> > 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 
> 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  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 
> > >> 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 
> > >> >>> > > To: dev@kafka.apache.org

Re: [DISCUSS] KIP-201: Rationalising Policy interfaces

[Mickael Maison]

 at 10:37, Ismael Juma  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 
> >> >>> 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 
> >> 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 
> >> >>> > > 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.
> >> >>> > >
> >&

Re: [DISCUSS] KIP-201: Rationalising Policy interfaces

[Anna Povzner]

s,
>
> Anna
>
>
> On Mon, Oct 9, 2017 at 1:39 AM, Tom Bentley  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  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  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  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 
>> >>> 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.
>> >>> >
>> >>> &

Re: [DISCUSS] KIP-201: Rationalising Policy interfaces

[Anna Povzner]

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

Re: [DISCUSS] KIP-201: Rationalising Policy interfaces

[Tom Bentley]

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).
>>> > >
>>> &g

Re: [DISCUSS] KIP-201: Rationalising Policy interfaces

[Tom Bentley]

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
>

Re: [DISCUSS] KIP-201: Rationalising Policy interfaces

[Tom Bentley]

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=DwIBaQ=jf_iaSHvJObTbx-siA1ZOg=
> > > EzRhmSah4IHsUZVekRUIINhltZK7U0OaeRo7hgW4_tQ=h-D-nA7uiy1Z-jta5y-
> > > yh7dKgV77X

Re: [DISCUSS] KIP-201: Rationalising Policy interfaces

[Ismael Juma]

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=DwIBaQ=jf_iaSHvJObTbx-siA1ZOg=
> > EzRhmSah4IHsUZVekRUIINhltZK7U0OaeRo7hgW4_tQ=h-D-nA7uiy1Z-jta5y-
> > yh7dKgV77XtsUnJ9Rab1gheY=43hzTLEDKw2v5Vh0zwkMTaaKD-
> HdJD8d_F4-Bsw25-Y=
> > >
> > > Linkedin : paolopatierno<
> > https://urldefense.proofpoint.com/v2/url?u=http-3A__it.
> > linkedin.com_in_paolopatierno=DwIBaQ=jf_iaSHvJObTbx-siA1ZOg=
> > EzRhmSah4IHsUZVekRUIINhltZK7U0OaeRo7hgW4_tQ=h-D-nA7uiy1Z-jta5y-
> > yh7dKgV77XtsUnJ9Rab1gheY=Ig0N7Nwf9EHfTJ2pH3jRM1JIdlzXw6
> R5Drocu0TMRLk=
> > >
> > > Blog : DevExperience<
> > https://urldefense.proofpoint.com/v2/url?u=http-3A__
> > paolopatierno.wordpress.com_=DwIBaQ=jf_iaSHvJObTbx-siA1ZOg=
> > EzRhmSah4IHsUZVekRUIINhltZK7U0OaeRo7hgW4_tQ=h-D-nA7uiy1Z-jta5y-
> > yh7dKgV77XtsUnJ9Rab1gheY=Tc9NrTtG2GP7-zRjOHkXHfYI0rncO8_
> jKpedna692z4=
> > >
> &

Re: [DISCUSS] KIP-201: Rationalising Policy interfaces

[Tom Bentley]

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=DwIBaQ=jf_iaSHvJObTbx-siA1ZOg=
> EzRhmSah4IHsUZVekRUIINhltZK7U0OaeRo7hgW4_tQ=h-D-nA7uiy1Z-jta5y-
> yh7dKgV77XtsUnJ9Rab1gheY=43hzTLEDKw2v5Vh0zwkMTaaKD-HdJD8d_F4-Bsw25-Y=
> >
> > Linkedin : paolopatierno<
> https://urldefense.proofpoint.com/v2/url?u=http-3A__it.
> linkedin.com_in_paolopatierno=DwIBaQ=jf_iaSHvJObTbx-siA1ZOg=
> EzRhmSah4IHsUZVekRUIINhltZK7U0OaeRo7hgW4_tQ=h-D-nA7uiy1Z-jta5y-
> yh7dKgV77XtsUnJ9Rab1gheY=Ig0N7Nwf9EHfTJ2pH3jRM1JIdlzXw6R5Drocu0TMRLk=
> >
> > Blog : DevExperience<
> https://urldefense.proofpoint.com/v2/url?u=http-3A__
> paolopatierno.wordpress.com_=DwIBaQ=jf_iaSHvJObTbx-siA1ZOg=
> EzRhmSah4IHsUZVekRUIINhltZK7U0OaeRo7hgW4_tQ=h-D-nA7uiy1Z-jta5y-
> yh7dKgV77XtsUnJ9Rab1gheY=Tc9NrTtG2GP7-zRjOHkXHfYI0rncO8_jKpedna692z4=
> >
> >
> >
> > 
> > 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>

Re: [DISCUSS] KIP-201: Rationalising Policy interfaces

[Edoardo Comar]

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=DwIBaQ=jf_iaSHvJObTbx-siA1ZOg=EzRhmSah4IHsUZVekRUIINhltZK7U0OaeRo7hgW4_tQ=h-D-nA7uiy1Z-jta5y-yh7dKgV77XtsUnJ9Rab1gheY=43hzTLEDKw2v5Vh0zwkMTaaKD-HdJD8d_F4-Bsw25-Y=
 
>
> Linkedin : paolopatierno<
https://urldefense.proofpoint.com/v2/url?u=http-3A__it.linkedin.com_in_paolopatierno=DwIBaQ=jf_iaSHvJObTbx-siA1ZOg=EzRhmSah4IHsUZVekRUIINhltZK7U0OaeRo7hgW4_tQ=h-D-nA7uiy1Z-jta5y-yh7dKgV77XtsUnJ9Rab1gheY=Ig0N7Nwf9EHfTJ2pH3jRM1JIdlzXw6R5Drocu0TMRLk=
 
>
> Blog : DevExperience<
https://urldefense.proofpoint.com/v2/url?u=http-3A__paolopatierno.wordpress.com_=DwIBaQ=jf_iaSHvJObTbx-siA1ZOg=EzRhmSah4IHsUZVekRUIINhltZK7U0OaeRo7hgW4_tQ=h-D-nA7uiy1Z-jta5y-yh7dKgV77XtsUnJ9Rab1gheY=Tc9NrTtG2GP7-zRjOHkXHfYI0rncO8_jKpedna692z4=
 
>
>
>
> 
> 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_p

Re: [DISCUSS] KIP-201: Rationalising Policy interfaces

[Tom Bentley]

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<http://twitter.com/ppatierno>
> Linkedin : paolopatierno<http://it.linkedin.com/in/paolopatierno>
> Blog : DevExperience<http://paolopatierno.wordpress.com/>
>
>
> 
> 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<http://twitter.com/ppatierno>
> > Linkedin : paolopatierno<http://it.linkedin.com/in/paolopatierno>
> > Blog : DevExperience<http://paolopatierno.wordpress.com/>
> >
> >
> > 
> > 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 t

Re: [DISCUSS] KIP-201: Rationalising Policy interfaces

[Paolo Patierno]

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<http://twitter.com/ppatierno>
Linkedin : paolopatierno<http://it.linkedin.com/in/paolopatierno>
Blog : DevExperience<http://paolopatierno.wordpress.com/>



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<http://twitter.com/ppatierno>
> Linkedin : paolopatierno<http://it.linkedin.com/in/paolopatierno>
> Blog : DevExperience<http://paolopatierno.wordpress.com/>
>
>
> 
> 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
>

Re: [DISCUSS] KIP-201: Rationalising Policy interfaces

[Tom Bentley]

Hi Ismael,

Thanks for looking at the KIP and explaining the thinking behind the
original API.

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

I can certainly see that point of view. I wasn't too happy with trying to
shoehorn deletion in with creation and alteration though. It seems to me
that the request metadata are different. One alternative would be to have a
single policy interface (TopicManagementPolicy) with different methods for
the create/alter and delete cases, with the delete having a different
request metadata type.

Since Java 7 is no longer supported we could have a different policy method
per request. Using default interface methods means that adding new policy
methods wouldn't be a breaking change. That said I still think having a
common method for topic creation and alteration makes more sense: It forces
the implementer of the interface to consider both cases at the same time.

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

Yes, sorry, seems I've managed to misunderstand the policy again. It would
be really helpful if the policy were written down somewhere (though I
realise that since 1.0.0 is not out we might not want to publish it yet).
Anyway, I'll update the KIP.

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

I realise they're not the exactly the same, but in the end it's the
messages which have value to the business running a cluster and the policy
around losing those messages is likely to be the same which ever way they
might be deleted.

Thanks again,

Tom

Re: [DISCUSS] KIP-201: Rationalising Policy interfaces

[Ismael Juma]

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<http://twitter.com/ppatierno>
> Linkedin : paolopatierno<http://it.linkedin.com/in/paolopatierno>
> Blog : DevExperience<http://paolopatierno.wordpress.com/>
>
>
> 
> 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 topics();
> >> > }
> >> >
> >> > I think that the implementation of ClusterState that

Re: [DISCUSS] KIP-201: Rationalising Policy interfaces

[Paolo Patierno]

Hi Tom,


I guess that at


"On topic deletion will be applied on topic and message deletion."


you meant something like "The TopicDeletionPolicy will be applied on topic and 
messages deletion".


Paolo Patierno
Senior Software Engineer (IoT) @ Red Hat
Microsoft MVP on Azure & IoT
Microsoft Azure Advisor

Twitter : @ppatierno<http://twitter.com/ppatierno>
Linkedin : paolopatierno<http://it.linkedin.com/in/paolopatierno>
Blog : DevExperience<http://paolopatierno.wordpress.com/>



From: Tom Bentley <t.j.bent...@gmail.com>
Sent: Wednesday, September 27, 2017 9:47 AM
To: dev@kafka.apache.org
Subject: Re: [DISCUSS] KIP-201: Rationalising Policy interfaces

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

Re: [DISCUSS] KIP-201: Rationalising Policy interfaces

[Paolo Patierno]

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<http://twitter.com/ppatierno>
Linkedin : paolopatierno<http://it.linkedin.com/in/paolopatierno>
Blog : DevExperience<http://paolopatierno.wordpress.com/>



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

Re: [DISCUSS] KIP-201: Rationalising Policy interfaces

[Ismael Juma]

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

Re: [DISCUSS] KIP-201: Rationalising Policy interfaces

[Ismael Juma]

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

Re: [DISCUSS] KIP-201: Rationalising Policy interfaces

[Tom Bentley]

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

Re: [DISCUSS] KIP-201: Rationalising Policy interfaces

[Edoardo Comar]

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

Re: [DISCUSS] KIP-201: Rationalising Policy interfaces

[Tom Bentley]

Hi Edoardo,


what about a single method in ClusterState
>
> interface ClusterState {
> public Map 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

Re: [DISCUSS] KIP-201: Rationalising Policy interfaces

[Paolo Patierno]

If we want to use the same DeleteTopicPolicy for the KIP-170 and for the delete 
records (in the topic) proposed by KIP-204, I think that we need more 
information other than topic name.

As I mentioned in the KIP-204 discussion, the user could be interested in 
having more information because the delete records takes info like partition 
and specific offset.


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: Edoardo Comar <eco...@uk.ibm.com>
Sent: Tuesday, September 26, 2017 4:18 PM
To: dev@kafka.apache.org
Subject: Re: [DISCUSS] KIP-201: Rationalising Policy interfaces

Thanks Tom,

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 ?

--

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 16:41
Subject:    Re: [DISCUSS] KIP-201: Rationalising Policy interfaces



Hi Edoardo and Ismael,

Thanks for the comments. If we're going to have an new ClusterState
parameter via which we can query the cluster state it would make sense to
obtain the existing state  from the ClusterState than from the
RequestMetadata as in the KIP up to now. So I've updated the KIP along
these lines, but there are still a couple of issues:

1 ClusterState.topicsPartitionCount() isn't really necessary, since the
same information is available by repeated calls to
ClusterState.topicState().
We still need a way to get the topics in the cluster, but maybe the method
would allow including/excluding the internal and marked-for-deletion
topics
from the result.
2. Should the TopicState include whether the topic is an internal topic?
3. Do we really need TopicState at all? It would imply creating a new
instance for each topic queried. We could avoid that by pushing the
methods
into ClusterState and RequestMetadata. Since I don't think we need to
abstract over TopicStates, I'm minded to do this.

Cheers,

Tom



On 26 September 2017 at 14:59, Edoardo Comar <eco...@uk.ibm.com> wrote:

> Furthermore, the deletion case could be implemented by a custom
> Authorizer, as discussed in the KIP-204 thread.
> --
>
> Edoardo Comar
>
> IBM Message Hub
>
> IBM UK Ltd, Hursley Park, SO21 2JN
>
>
>
> From:   Edoardo Comar <eco...@uk.ibm.com>
> To: dev@kafka.apache.org
> Date:   26/09/2017 14:01
> Subject:Re: [DISCUSS] KIP-201: Rationalising Policy interfaces
>
>
>
> Hi Tom,
> yes it makes sense to have a single KIP for enhancing these policies.
>
> As Mickael pointed out, we need that the create/alter topic policy are
> able to assess the current cluster metadata.
> KIP-170 suggested a Provider interface with the minimal set of methods
> that we needed.
>
> However Ismael on 22/06/2017 suggested changes to KIP-170 that i didn't
> manage to incorporate yet there.
> instead of a provider interface Ismael suggested to extend the
> RequestMetadata :
>
> > Thanks for the KIP, Edoardo. A few comments:
> >
> > 1. Have you considered extending RequestMetadata with the additional
> >information you need? We could add Cluster to it, which has topic
> >assignment information, for example. This way, there would be no need
for
>
> a
> V2 interface.
>
> >2. Something else that could be useful is passing an instance of
> `Session`
> >so that one can provide custom behaviour depending on the logged in
user.
> >Would this be useful?
>
> > 3. For the delete case, we may consider passing a class instead of
just
> a
> > string to the validate method so that we have options if we need to
> extend
> >it.
>
> > 4. Do we want to enhance the AlterConfigs policy as well?
> >
> > Ismael
>
>
> His change proposal #1 would be fine with me - although I do not see how
> we could check if isTopicMarkedForDeletion.
> as for change #2 having the KafkaPrincipal instead of the session works
> for me
>
> As for the delete policy - our motivation is entirely different : we
> needed a policy essentially to ensure that the topic was not registered
> with dependent services that we did not want to break.  For a delete
> policy to be usable in a friendly way, the Kafka protocol needs to be
> updated as in KIP-170 to

Re: [DISCUSS] KIP-201: Rationalising Policy interfaces

[Edoardo Comar]

Thanks Tom,

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 ?

--

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 16:41
Subject:    Re: [DISCUSS] KIP-201: Rationalising Policy interfaces



Hi Edoardo and Ismael,

Thanks for the comments. If we're going to have an new ClusterState
parameter via which we can query the cluster state it would make sense to
obtain the existing state  from the ClusterState than from the
RequestMetadata as in the KIP up to now. So I've updated the KIP along
these lines, but there are still a couple of issues:

1 ClusterState.topicsPartitionCount() isn't really necessary, since the
same information is available by repeated calls to 
ClusterState.topicState().
We still need a way to get the topics in the cluster, but maybe the method
would allow including/excluding the internal and marked-for-deletion 
topics
from the result.
2. Should the TopicState include whether the topic is an internal topic?
3. Do we really need TopicState at all? It would imply creating a new
instance for each topic queried. We could avoid that by pushing the 
methods
into ClusterState and RequestMetadata. Since I don't think we need to
abstract over TopicStates, I'm minded to do this.

Cheers,

Tom



On 26 September 2017 at 14:59, Edoardo Comar <eco...@uk.ibm.com> wrote:

> Furthermore, the deletion case could be implemented by a custom
> Authorizer, as discussed in the KIP-204 thread.
> --
>
> Edoardo Comar
>
> IBM Message Hub
>
> IBM UK Ltd, Hursley Park, SO21 2JN
>
>
>
> From:   Edoardo Comar <eco...@uk.ibm.com>
> To: dev@kafka.apache.org
> Date:   26/09/2017 14:01
> Subject:Re: [DISCUSS] KIP-201: Rationalising Policy interfaces
>
>
>
> Hi Tom,
> yes it makes sense to have a single KIP for enhancing these policies.
>
> As Mickael pointed out, we need that the create/alter topic policy are
> able to assess the current cluster metadata.
> KIP-170 suggested a Provider interface with the minimal set of methods
> that we needed.
>
> However Ismael on 22/06/2017 suggested changes to KIP-170 that i didn't
> manage to incorporate yet there.
> instead of a provider interface Ismael suggested to extend the
> RequestMetadata :
>
> > Thanks for the KIP, Edoardo. A few comments:
> >
> > 1. Have you considered extending RequestMetadata with the additional
> >information you need? We could add Cluster to it, which has topic
> >assignment information, for example. This way, there would be no need 
for
>
> a
> V2 interface.
>
> >2. Something else that could be useful is passing an instance of
> `Session`
> >so that one can provide custom behaviour depending on the logged in 
user.
> >Would this be useful?
>
> > 3. For the delete case, we may consider passing a class instead of 
just
> a
> > string to the validate method so that we have options if we need to
> extend
> >it.
>
> > 4. Do we want to enhance the AlterConfigs policy as well?
> >
> > Ismael
>
>
> His change proposal #1 would be fine with me - although I do not see how
> we could check if isTopicMarkedForDeletion.
> as for change #2 having the KafkaPrincipal instead of the session works
> for me
>
> As for the delete policy - our motivation is entirely different : we
> needed a policy essentially to ensure that the topic was not registered
> with dependent services that we did not want to break.  For a delete
> policy to be usable in a friendly way, the Kafka protocol needs to be
> updated as in KIP-170 to include a message in the delete topic response
> (to tell why it's failed).
>
> I'm happy if you incorporate the enhancements to create/alter that allow 
a
>
> check against the cluster metadata
> and leave out - to anther KIP, or maybe I'll rewrite 170 the changes to
> delete.
>
> 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:   25/09/2017 18:11
> Subject:Re: [DISCUSS] KIP-201: Rationalising Policy interfaces
>
>
>
> Hi Mickael,
>
> Thanks for the reply.
>
> Thanks for the KIP. Is this meant to superseed KIP-170 ?
> > If so, one of our key requirements was to be able to access the
> > topics/partitions list from t

Re: [DISCUSS] KIP-201: Rationalising Policy interfaces

[Tom Bentley]

Hi Edoardo and Ismael,

Thanks for the comments. If we're going to have an new ClusterState
parameter via which we can query the cluster state it would make sense to
obtain the existing state  from the ClusterState than from the
RequestMetadata as in the KIP up to now. So I've updated the KIP along
these lines, but there are still a couple of issues:

1 ClusterState.topicsPartitionCount() isn't really necessary, since the
same information is available by repeated calls to ClusterState.topicState().
We still need a way to get the topics in the cluster, but maybe the method
would allow including/excluding the internal and marked-for-deletion topics
from the result.
2. Should the TopicState include whether the topic is an internal topic?
3. Do we really need TopicState at all? It would imply creating a new
instance for each topic queried. We could avoid that by pushing the methods
into ClusterState and RequestMetadata. Since I don't think we need to
abstract over TopicStates, I'm minded to do this.

Cheers,

Tom



On 26 September 2017 at 14:59, Edoardo Comar <eco...@uk.ibm.com> wrote:

> Furthermore, the deletion case could be implemented by a custom
> Authorizer, as discussed in the KIP-204 thread.
> --
>
> Edoardo Comar
>
> IBM Message Hub
>
> IBM UK Ltd, Hursley Park, SO21 2JN
>
>
>
> From:   Edoardo Comar <eco...@uk.ibm.com>
> To: dev@kafka.apache.org
> Date:   26/09/2017 14:01
> Subject:    Re: [DISCUSS] KIP-201: Rationalising Policy interfaces
>
>
>
> Hi Tom,
> yes it makes sense to have a single KIP for enhancing these policies.
>
> As Mickael pointed out, we need that the create/alter topic policy are
> able to assess the current cluster metadata.
> KIP-170 suggested a Provider interface with the minimal set of methods
> that we needed.
>
> However Ismael on 22/06/2017 suggested changes to KIP-170 that i didn't
> manage to incorporate yet there.
> instead of a provider interface Ismael suggested to extend the
> RequestMetadata :
>
> > Thanks for the KIP, Edoardo. A few comments:
> >
> > 1. Have you considered extending RequestMetadata with the additional
> >information you need? We could add Cluster to it, which has topic
> >assignment information, for example. This way, there would be no need for
>
> a
> V2 interface.
>
> >2. Something else that could be useful is passing an instance of
> `Session`
> >so that one can provide custom behaviour depending on the logged in user.
> >Would this be useful?
>
> > 3. For the delete case, we may consider passing a class instead of just
> a
> > string to the validate method so that we have options if we need to
> extend
> >it.
>
> > 4. Do we want to enhance the AlterConfigs policy as well?
> >
> > Ismael
>
>
> His change proposal #1 would be fine with me - although I do not see how
> we could check if isTopicMarkedForDeletion.
> as for change #2 having the KafkaPrincipal instead of the session works
> for me
>
> As for the delete policy - our motivation is entirely different : we
> needed a policy essentially to ensure that the topic was not registered
> with dependent services that we did not want to break.  For a delete
> policy to be usable in a friendly way, the Kafka protocol needs to be
> updated as in KIP-170 to include a message in the delete topic response
> (to tell why it's failed).
>
> I'm happy if you incorporate the enhancements to create/alter that allow a
>
> check against the cluster metadata
> and leave out - to anther KIP, or maybe I'll rewrite 170 the changes to
> delete.
>
> 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:   25/09/2017 18:11
> Subject:Re: [DISCUSS] KIP-201: Rationalising Policy interfaces
>
>
>
> Hi Mickael,
>
> Thanks for the reply.
>
> Thanks for the KIP. Is this meant to superseed KIP-170 ?
> > If so, one of our key requirements was to be able to access the
> > topics/partitions list from the policy, so an administrator could
> > enforce a partition limit for example.
> >
>
> It's not meant to replace KIP-170 because there are things in KIP-170
> which
> aren't purely about policy (the change in requests and responses, for
> example), which KIP-201 doesn't propose to implement. Obviously there is
> overlap when it comes to policies, and both KIPs start with different
> motivations for the policy changes they propose. I think it makes sense
> for
> a single KIP address

Re: [DISCUSS] KIP-201: Rationalising Policy interfaces

[Edoardo Comar]

Furthermore, the deletion case could be implemented by a custom 
Authorizer, as discussed in the KIP-204 thread.
--

Edoardo Comar

IBM Message Hub

IBM UK Ltd, Hursley Park, SO21 2JN



From:   Edoardo Comar <eco...@uk.ibm.com>
To: dev@kafka.apache.org
Date:   26/09/2017 14:01
Subject:        Re: [DISCUSS] KIP-201: Rationalising Policy interfaces



Hi Tom,
yes it makes sense to have a single KIP for enhancing these policies.

As Mickael pointed out, we need that the create/alter topic policy are 
able to assess the current cluster metadata.
KIP-170 suggested a Provider interface with the minimal set of methods 
that we needed.

However Ismael on 22/06/2017 suggested changes to KIP-170 that i didn't 
manage to incorporate yet there.
instead of a provider interface Ismael suggested to extend the 
RequestMetadata :

> Thanks for the KIP, Edoardo. A few comments:
> 
> 1. Have you considered extending RequestMetadata with the additional
>information you need? We could add Cluster to it, which has topic
>assignment information, for example. This way, there would be no need for 

a
V2 interface.

>2. Something else that could be useful is passing an instance of 
`Session`
>so that one can provide custom behaviour depending on the logged in user.
>Would this be useful?

> 3. For the delete case, we may consider passing a class instead of just 
a
> string to the validate method so that we have options if we need to 
extend
>it.

> 4. Do we want to enhance the AlterConfigs policy as well?
>
> Ismael


His change proposal #1 would be fine with me - although I do not see how 
we could check if isTopicMarkedForDeletion.
as for change #2 having the KafkaPrincipal instead of the session works 
for me

As for the delete policy - our motivation is entirely different : we 
needed a policy essentially to ensure that the topic was not registered 
with dependent services that we did not want to break.  For a delete 
policy to be usable in a friendly way, the Kafka protocol needs to be 
updated as in KIP-170 to include a message in the delete topic response 
(to tell why it's failed).

I'm happy if you incorporate the enhancements to create/alter that allow a 

check against the cluster metadata 
and leave out - to anther KIP, or maybe I'll rewrite 170 the changes to 
delete.

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:   25/09/2017 18:11
Subject:Re: [DISCUSS] KIP-201: Rationalising Policy interfaces



Hi Mickael,

Thanks for the reply.

Thanks for the KIP. Is this meant to superseed KIP-170 ?
> If so, one of our key requirements was to be able to access the
> topics/partitions list from the policy, so an administrator could
> enforce a partition limit for example.
>

It's not meant to replace KIP-170 because there are things in KIP-170 
which
aren't purely about policy (the change in requests and responses, for
example), which KIP-201 doesn't propose to implement. Obviously there is
overlap when it comes to policies, and both KIPs start with different
motivations for the policy changes they propose. I think it makes sense 
for
a single KIP address both sets of use cases if possible. I'm happy for 
that
to be KIP-201 if that suits you.

I think the approach taken in KIP-170, of a Provider interface for
obtaining information that's not intrinsic to the request and a method to
inject that provider into the policy, is a good one. It retains a clean
distinction between the request metadata itself and the wider cluster
metadata, which I think is a good thing. If you're happy Mickael, I'll
update KIP-201 with something similar.


> Also instead of simply having the Java Principal object, could we have
> the KafkaPrincipal ? So policies could take advantage of custom
> KafkaPrincipal object (KIP-189).
>

Certainly.



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

Re: [DISCUSS] KIP-201: Rationalising Policy interfaces

[Ismael Juma]

If we are introducing another interface, I think it makes sense to
complement the request metadata with a context class that exposes
non-request information that may be useful. I'll hopefully review the KIP
in more detail today or tomorrow.

Ismael

On Tue, Sep 26, 2017 at 2:01 PM, Edoardo Comar <eco...@uk.ibm.com> wrote:

> Hi Tom,
> yes it makes sense to have a single KIP for enhancing these policies.
>
> As Mickael pointed out, we need that the create/alter topic policy are
> able to assess the current cluster metadata.
> KIP-170 suggested a Provider interface with the minimal set of methods
> that we needed.
>
> However Ismael on 22/06/2017 suggested changes to KIP-170 that i didn't
> manage to incorporate yet there.
> instead of a provider interface Ismael suggested to extend the
> RequestMetadata :
>
> > Thanks for the KIP, Edoardo. A few comments:
> >
> > 1. Have you considered extending RequestMetadata with the additional
> >information you need? We could add Cluster to it, which has topic
> >assignment information, for example. This way, there would be no need for
> a
> V2 interface.
>
> >2. Something else that could be useful is passing an instance of
> `Session`
> >so that one can provide custom behaviour depending on the logged in user.
> >Would this be useful?
>
> > 3. For the delete case, we may consider passing a class instead of just
> a
> > string to the validate method so that we have options if we need to
> extend
> >it.
>
> > 4. Do we want to enhance the AlterConfigs policy as well?
> >
> > Ismael
>
>
> His change proposal #1 would be fine with me - although I do not see how
> we could check if isTopicMarkedForDeletion.
> as for change #2 having the KafkaPrincipal instead of the session works
> for me
>
> As for the delete policy - our motivation is entirely different : we
> needed a policy essentially to ensure that the topic was not registered
> with dependent services that we did not want to break.  For a delete
> policy to be usable in a friendly way, the Kafka protocol needs to be
> updated as in KIP-170 to include a message in the delete topic response
> (to tell why it's failed).
>
> I'm happy if you incorporate the enhancements to create/alter that allow a
> check against the cluster metadata
> and leave out - to anther KIP, or maybe I'll rewrite 170 the changes to
> delete.
>
> 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:   25/09/2017 18:11
> Subject:Re: [DISCUSS] KIP-201: Rationalising Policy interfaces
>
>
>
> Hi Mickael,
>
> Thanks for the reply.
>
> Thanks for the KIP. Is this meant to superseed KIP-170 ?
> > If so, one of our key requirements was to be able to access the
> > topics/partitions list from the policy, so an administrator could
> > enforce a partition limit for example.
> >
>
> It's not meant to replace KIP-170 because there are things in KIP-170
> which
> aren't purely about policy (the change in requests and responses, for
> example), which KIP-201 doesn't propose to implement. Obviously there is
> overlap when it comes to policies, and both KIPs start with different
> motivations for the policy changes they propose. I think it makes sense
> for
> a single KIP address both sets of use cases if possible. I'm happy for
> that
> to be KIP-201 if that suits you.
>
> I think the approach taken in KIP-170, of a Provider interface for
> obtaining information that's not intrinsic to the request and a method to
> inject that provider into the policy, is a good one. It retains a clean
> distinction between the request metadata itself and the wider cluster
> metadata, which I think is a good thing. If you're happy Mickael, I'll
> update KIP-201 with something similar.
>
>
> > Also instead of simply having the Java Principal object, could we have
> > the KafkaPrincipal ? So policies could take advantage of custom
> > KafkaPrincipal object (KIP-189).
> >
>
> Certainly.
>
>
>
> 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
>

Re: [DISCUSS] KIP-201: Rationalising Policy interfaces

[Edoardo Comar]

Hi Tom,
yes it makes sense to have a single KIP for enhancing these policies.

As Mickael pointed out, we need that the create/alter topic policy are 
able to assess the current cluster metadata.
KIP-170 suggested a Provider interface with the minimal set of methods 
that we needed.

However Ismael on 22/06/2017 suggested changes to KIP-170 that i didn't 
manage to incorporate yet there.
instead of a provider interface Ismael suggested to extend the 
RequestMetadata :

> Thanks for the KIP, Edoardo. A few comments:
> 
> 1. Have you considered extending RequestMetadata with the additional
>information you need? We could add Cluster to it, which has topic
>assignment information, for example. This way, there would be no need for 
a
V2 interface.

>2. Something else that could be useful is passing an instance of 
`Session`
>so that one can provide custom behaviour depending on the logged in user.
>Would this be useful?

> 3. For the delete case, we may consider passing a class instead of just 
a
> string to the validate method so that we have options if we need to 
extend
>it.

> 4. Do we want to enhance the AlterConfigs policy as well?
>
> Ismael


His change proposal #1 would be fine with me - although I do not see how 
we could check if isTopicMarkedForDeletion.
as for change #2 having the KafkaPrincipal instead of the session works 
for me

As for the delete policy - our motivation is entirely different : we 
needed a policy essentially to ensure that the topic was not registered 
with dependent services that we did not want to break.  For a delete 
policy to be usable in a friendly way, the Kafka protocol needs to be 
updated as in KIP-170 to include a message in the delete topic response 
(to tell why it's failed).

I'm happy if you incorporate the enhancements to create/alter that allow a 
check against the cluster metadata 
and leave out - to anther KIP, or maybe I'll rewrite 170 the changes to 
delete.

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:   25/09/2017 18:11
Subject:    Re: [DISCUSS] KIP-201: Rationalising Policy interfaces



Hi Mickael,

Thanks for the reply.

Thanks for the KIP. Is this meant to superseed KIP-170 ?
> If so, one of our key requirements was to be able to access the
> topics/partitions list from the policy, so an administrator could
> enforce a partition limit for example.
>

It's not meant to replace KIP-170 because there are things in KIP-170 
which
aren't purely about policy (the change in requests and responses, for
example), which KIP-201 doesn't propose to implement. Obviously there is
overlap when it comes to policies, and both KIPs start with different
motivations for the policy changes they propose. I think it makes sense 
for
a single KIP address both sets of use cases if possible. I'm happy for 
that
to be KIP-201 if that suits you.

I think the approach taken in KIP-170, of a Provider interface for
obtaining information that's not intrinsic to the request and a method to
inject that provider into the policy, is a good one. It retains a clean
distinction between the request metadata itself and the wider cluster
metadata, which I think is a good thing. If you're happy Mickael, I'll
update KIP-201 with something similar.


> Also instead of simply having the Java Principal object, could we have
> the KafkaPrincipal ? So policies could take advantage of custom
> KafkaPrincipal object (KIP-189).
>

Certainly.



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

Re: [DISCUSS] KIP-201: Rationalising Policy interfaces

[Tom Bentley]

Hi Ismael,

On 25 September 2017 at 17:51, Ismael Juma  wrote:

> We don't have this policy today for what it's worth.
>

Thanks for the clarification. On re-reading I realise I misinterpreted
Guozhang Wang's suggestion when 1.0.0 was first mooted:


> Just to clarify, my proposal is that moving forward beyond the next release
> we will not make any public API breaking changes in any of the major or
> minor releases, but will only mark them as "deprecated", and deprecated
> public APIs will be only considered for removing as early as the next major
> release: so if we mark the scala consumer APIs as deprecated in 1.0.0, we
> should only be consider removing it at 2.0.0 or even later.


So this would mean that if a deprecation got into 1.x the feature could be
removed in 1.(x+1) at the earliest, right? I will update the KIP.

btw isn't this a point which should be included in
https://issues.apache.org/jira/browse/KAFKA-5637 ?

Thanks,

Tom

Re: [DISCUSS] KIP-201: Rationalising Policy interfaces

[Tom Bentley]

Hi Mickael,

Thanks for the reply.

Thanks for the KIP. Is this meant to superseed KIP-170 ?
> If so, one of our key requirements was to be able to access the
> topics/partitions list from the policy, so an administrator could
> enforce a partition limit for example.
>

It's not meant to replace KIP-170 because there are things in KIP-170 which
aren't purely about policy (the change in requests and responses, for
example), which KIP-201 doesn't propose to implement. Obviously there is
overlap when it comes to policies, and both KIPs start with different
motivations for the policy changes they propose. I think it makes sense for
a single KIP address both sets of use cases if possible. I'm happy for that
to be KIP-201 if that suits you.

I think the approach taken in KIP-170, of a Provider interface for
obtaining information that's not intrinsic to the request and a method to
inject that provider into the policy, is a good one. It retains a clean
distinction between the request metadata itself and the wider cluster
metadata, which I think is a good thing. If you're happy Mickael, I'll
update KIP-201 with something similar.


> Also instead of simply having the Java Principal object, could we have
> the KafkaPrincipal ? So policies could take advantage of custom
> KafkaPrincipal object (KIP-189).
>

Certainly.

Re: [DISCUSS] KIP-201: Rationalising Policy interfaces

[Ismael Juma]

On Mon, Sep 25, 2017 at 5:32 PM, Tom Bentley  wrote:

> > bq. If this KIP is accepted for Kafka 1.1.0 this removal could happen in
> > Kafka 3.0.0
> >
> > There would be no Kafka 2.0 ?
> >
>
> As I understand it, a deprecation has to exist for a complete major version
> number cycle before the feature can be removed. So deprecations that are
> added in 1.x (x>0) have to remain in all 2.y before removal in 3. Did I
> understand the policy wrong?
>

We don't have this policy today for what it's worth.

Ismael

Re: [DISCUSS] KIP-201: Rationalising Policy interfaces

[Mickael Maison]

Hi Tom,

Thanks for the KIP. Is this meant to superseed KIP-170 ?
If so, one of our key requirements was to be able to access the
topics/partitions list from the policy, so an administrator could
enforce a partition limit for example.

Also instead of simply having the Java Principal object, could we have
the KafkaPrincipal ? So policies could take advantage of custom
KafkaPrincipal object (KIP-189).

On Mon, Sep 25, 2017 at 5:32 PM, Tom Bentley  wrote:
> Hi Ted,
>
> Thanks for the feedback!
>
> bq. topic.action.policy.class.name
>>
>> Since the policy would cover more than one action, how about using actions
>> for the second word ?
>>
>
> Good point, done.
>
>
>> For TopicState interface, the abstract modifier for its methods are not
>> needed.
>>
>
> Fixed.
>
> bq. KIP-113
>>
>> Mind adding more to the above bullet ?
>>
>
> I guess I intended to elaborate on this, but forgot to. I guess the
> question is:
>
> a) Whether AlterReplicaDir should be covered by a policy, and if so
> b) should it be covered by this policy.
>
> Thinking about it some more I don't think it should be covered by this
> policy, so I have removed this bullet. Please shout if you disagree.
>
>
>> bq. If this KIP is accepted for Kafka 1.1.0 this removal could happen in
>> Kafka 3.0.0
>>
>> There would be no Kafka 2.0 ?
>>
>
> As I understand it, a deprecation has to exist for a complete major version
> number cycle before the feature can be removed. So deprecations that are
> added in 1.x (x>0) have to remain in all 2.y before removal in 3. Did I
> understand the policy wrong?

Re: [DISCUSS] KIP-201: Rationalising Policy interfaces

[Ted Yu]

bq. deprecations that are added in 1.x (x>0) have to remain in all 2.y

Makes sense.

It is fine to exclude KIP-113 from your KIP.

Thanks

On Mon, Sep 25, 2017 at 9:32 AM, Tom Bentley  wrote:

> Hi Ted,
>
> Thanks for the feedback!
>
> bq. topic.action.policy.class.name
> >
> > Since the policy would cover more than one action, how about using
> actions
> > for the second word ?
> >
>
> Good point, done.
>
>
> > For TopicState interface, the abstract modifier for its methods are not
> > needed.
> >
>
> Fixed.
>
> bq. KIP-113
> >
> > Mind adding more to the above bullet ?
> >
>
> I guess I intended to elaborate on this, but forgot to. I guess the
> question is:
>
> a) Whether AlterReplicaDir should be covered by a policy, and if so
> b) should it be covered by this policy.
>
> Thinking about it some more I don't think it should be covered by this
> policy, so I have removed this bullet. Please shout if you disagree.
>
>
> > bq. If this KIP is accepted for Kafka 1.1.0 this removal could happen in
> > Kafka 3.0.0
> >
> > There would be no Kafka 2.0 ?
> >
>
> As I understand it, a deprecation has to exist for a complete major version
> number cycle before the feature can be removed. So deprecations that are
> added in 1.x (x>0) have to remain in all 2.y before removal in 3. Did I
> understand the policy wrong?
>

Re: [DISCUSS] KIP-201: Rationalising Policy interfaces

[Tom Bentley]

Hi Ted,

Thanks for the feedback!

bq. topic.action.policy.class.name
>
> Since the policy would cover more than one action, how about using actions
> for the second word ?
>

Good point, done.


> For TopicState interface, the abstract modifier for its methods are not
> needed.
>

Fixed.

bq. KIP-113
>
> Mind adding more to the above bullet ?
>

I guess I intended to elaborate on this, but forgot to. I guess the
question is:

a) Whether AlterReplicaDir should be covered by a policy, and if so
b) should it be covered by this policy.

Thinking about it some more I don't think it should be covered by this
policy, so I have removed this bullet. Please shout if you disagree.


> bq. If this KIP is accepted for Kafka 1.1.0 this removal could happen in
> Kafka 3.0.0
>
> There would be no Kafka 2.0 ?
>

As I understand it, a deprecation has to exist for a complete major version
number cycle before the feature can be removed. So deprecations that are
added in 1.x (x>0) have to remain in all 2.y before removal in 3. Did I
understand the policy wrong?

Re: [DISCUSS] KIP-201: Rationalising Policy interfaces

[Ted Yu]

bq. topic.action.policy.class.name

Since the policy would cover more than one action, how about using actions
for the second word ?

For TopicState interface, the abstract modifier for its methods are not
needed.

bq. KIP-113

Mind adding more to the above bullet ?

bq. If this KIP is accepted for Kafka 1.1.0 this removal could happen in
Kafka 3.0.0

There would be no Kafka 2.0 ?

Cheers

On Mon, Sep 25, 2017 at 3:34 AM, Tom Bentley  wrote:

> Hi,
>
> I'd like to start a discussion for KIP-201. The basic point is that new
> AdminClient APIs for modifying topics should have a configurable policy to
> allow the administrator to veto the modifications. Just adding a
> ModifyTopicPolicy would make for awkwardness by having separate policies
> for creation and modification, so the KIP proposes unifying both of these
> under a single new policy.
>
> https://cwiki.apache.org/confluence/display/KAFKA/KIP-
> 201%3A+Rationalising+Policy+interfaces
>
> Cheers,
>
> Tom
>