You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@kafka.apache.org by Zhiguo Huang <je...@confluent.io> on 2020/05/05 20:04:05 UTC
[DISCUSS] KIP-608: Add a new method to AuthorizerServerInfo Interface
https://cwiki.apache.org/confluence/display/KAFKA/KIP-608+-+Add+a+new+method+to+AuthorizerServerInfo+Interface
Re: [DISCUSS] KIP-608: Add a new method to AuthorizerServerInfo Interface
Posted by Rajini Sivaram <ra...@gmail.com>.
Hi Jeff,
Thanks for the updated KIP, looks good.
Regards,
Rajini
On Fri, May 15, 2020 at 5:57 PM Jeff Huang <je...@confluent.io> wrote:
> Hi David,
>
> Thanks for your suggestions. I updated KIP based on your comments 1,2 and
> 4. For comment 3), I think the approach which Authorizer extend Monitorable
> would be simple for broker calling monitor function without checking
> whether Object implemented Monitorable function or not and make it
> generalization. This will also comply with convention like we do in other
> interface like Configurable.
>
> Thanks!
>
> Jeff.
>
> On 2020/05/15 15:09:36, David Jacot <dj...@confluent.io> wrote:
> > Hey Jeff,
> >
> > Thanks for the updated KIP. The interface looks good to me. I've got
> couple
> > of comments:
> >
> > 1. You say: "Other broker or client plugins could potentially implement
> the
> > interface and get the
> > broker's or client's Metrics instance to add additional metrics from
> > sub-components." Does it
> > imply that you will update the loading mechanism of all plugins? It won't
> > work otherwise. We should
> > do it probably as it is centralized. If not, we should clarify that we
> use
> > an interface to keep this option
> > open but that we will do it in a future KIP.
> >
> > 2. I would add the previous proposal to the rejected alternatives to not
> > forget about it.
> >
> > 3. I wonder if we should extend the Authorizer interface with the
> > Monitorable one by default or keep it
> > as an option for the concrete plugin to use it or not. I don't have a
> > strong opinion on this one. The
> > rationale behind this would be to keep it for advanced users. I wonder
> what
> > you and others think about this.
> >
> > 4. Metrics are considered as part of the public interface. It would be
> > great if you could move the description
> > in that section.
> >
> > Best,
> > David
> >
> > On Tue, May 12, 2020 at 7:46 PM Jeff Huang <je...@confluent.io>
> wrote:
> >
> > > Hey David,
> > > I accepted your suggestion and add Monitorable interface. Thanks for
> great
> > > suggestion. Please review KIP-608 again and see any suggestion on name
> of
> > > function or other stuff.
> > >
> > > jeff.
> > >
> > > On 2020/05/11 15:16:30, David Jacot <dj...@confluent.io> wrote:
> > > > Hey,
> > > >
> > > > Thanks for the KIP.
> > > >
> > > > I think that having the possibilities to expose metrics in plugins
> such
> > > as
> > > > the authorizer in a
> > > > nice improvement. I do wonder if we could come up with a more
> generic way
> > > > to do this that
> > > > would apply to all plugins instead of having something specific for
> the
> > > > authorized.
> > > >
> > > > For instance, we have the `Configurable` interface that allows us to
> > > > configure a plugin. We
> > > > could perhaps think of having a `Monitorable` interface that would
> allow
> > > to
> > > > pass the Metrics
> > > > instance to the plugin. Have you thought about such an approach?
> > > >
> > > > Best,
> > > > David
> > > >
> > > > On Wed, May 6, 2020 at 6:43 PM Jeff Huang <je...@confluent.io>
> > > wrote:
> > > >
> > > > > Will update KIP.
> > > > > thanks!
> > > > >
> > > > > On 2020/05/06 15:09:53, Rajini Sivaram <ra...@gmail.com>
> > > wrote:
> > > > > > Hi Jeff,
> > > > > >
> > > > > > Thanks for the KIP. It looks useful since it allows authorizers
> to
> > > use
> > > > > the
> > > > > > broker's metrics instance. We could perhaps use this in
> > > AclAuthorizer to
> > > > > > generate authorizer metrics?
> > > > > >
> > > > > > Regards,
> > > > > >
> > > > > > Rajini
> > > > > >
> > > > > > On Tue, May 5, 2020 at 9:04 PM Zhiguo Huang <
> jeff.huang@confluent.io
> > > >
> > > > > wrote:
> > > > > >
> > > > > > >
> > > > > > >
> > > > >
> > >
> https://cwiki.apache.org/confluence/display/KAFKA/KIP-608+-+Add+a+new+method+to+AuthorizerServerInfo+Interface
> > > > > > >
> > > > > >
> > > > >
> > > >
> > >
> >
>
Re: [DISCUSS] KIP-608: Add a new method to AuthorizerServerInfo Interface
Posted by Jeff Huang <je...@confluent.io>.
Hi David,
Thanks for your suggestions. I updated KIP based on your comments 1,2 and 4. For comment 3), I think the approach which Authorizer extend Monitorable would be simple for broker calling monitor function without checking whether Object implemented Monitorable function or not and make it generalization. This will also comply with convention like we do in other interface like Configurable.
Thanks!
Jeff.
On 2020/05/15 15:09:36, David Jacot <dj...@confluent.io> wrote:
> Hey Jeff,
>
> Thanks for the updated KIP. The interface looks good to me. I've got couple
> of comments:
>
> 1. You say: "Other broker or client plugins could potentially implement the
> interface and get the
> broker's or client's Metrics instance to add additional metrics from
> sub-components." Does it
> imply that you will update the loading mechanism of all plugins? It won't
> work otherwise. We should
> do it probably as it is centralized. If not, we should clarify that we use
> an interface to keep this option
> open but that we will do it in a future KIP.
>
> 2. I would add the previous proposal to the rejected alternatives to not
> forget about it.
>
> 3. I wonder if we should extend the Authorizer interface with the
> Monitorable one by default or keep it
> as an option for the concrete plugin to use it or not. I don't have a
> strong opinion on this one. The
> rationale behind this would be to keep it for advanced users. I wonder what
> you and others think about this.
>
> 4. Metrics are considered as part of the public interface. It would be
> great if you could move the description
> in that section.
>
> Best,
> David
>
> On Tue, May 12, 2020 at 7:46 PM Jeff Huang <je...@confluent.io> wrote:
>
> > Hey David,
> > I accepted your suggestion and add Monitorable interface. Thanks for great
> > suggestion. Please review KIP-608 again and see any suggestion on name of
> > function or other stuff.
> >
> > jeff.
> >
> > On 2020/05/11 15:16:30, David Jacot <dj...@confluent.io> wrote:
> > > Hey,
> > >
> > > Thanks for the KIP.
> > >
> > > I think that having the possibilities to expose metrics in plugins such
> > as
> > > the authorizer in a
> > > nice improvement. I do wonder if we could come up with a more generic way
> > > to do this that
> > > would apply to all plugins instead of having something specific for the
> > > authorized.
> > >
> > > For instance, we have the `Configurable` interface that allows us to
> > > configure a plugin. We
> > > could perhaps think of having a `Monitorable` interface that would allow
> > to
> > > pass the Metrics
> > > instance to the plugin. Have you thought about such an approach?
> > >
> > > Best,
> > > David
> > >
> > > On Wed, May 6, 2020 at 6:43 PM Jeff Huang <je...@confluent.io>
> > wrote:
> > >
> > > > Will update KIP.
> > > > thanks!
> > > >
> > > > On 2020/05/06 15:09:53, Rajini Sivaram <ra...@gmail.com>
> > wrote:
> > > > > Hi Jeff,
> > > > >
> > > > > Thanks for the KIP. It looks useful since it allows authorizers to
> > use
> > > > the
> > > > > broker's metrics instance. We could perhaps use this in
> > AclAuthorizer to
> > > > > generate authorizer metrics?
> > > > >
> > > > > Regards,
> > > > >
> > > > > Rajini
> > > > >
> > > > > On Tue, May 5, 2020 at 9:04 PM Zhiguo Huang <jeff.huang@confluent.io
> > >
> > > > wrote:
> > > > >
> > > > > >
> > > > > >
> > > >
> > https://cwiki.apache.org/confluence/display/KAFKA/KIP-608+-+Add+a+new+method+to+AuthorizerServerInfo+Interface
> > > > > >
> > > > >
> > > >
> > >
> >
>
Re: [DISCUSS] KIP-608: Add a new method to AuthorizerServerInfo Interface
Posted by David Jacot <dj...@confluent.io>.
Hey Jeff,
Thanks for the updated KIP. The interface looks good to me. I've got couple
of comments:
1. You say: "Other broker or client plugins could potentially implement the
interface and get the
broker's or client's Metrics instance to add additional metrics from
sub-components." Does it
imply that you will update the loading mechanism of all plugins? It won't
work otherwise. We should
do it probably as it is centralized. If not, we should clarify that we use
an interface to keep this option
open but that we will do it in a future KIP.
2. I would add the previous proposal to the rejected alternatives to not
forget about it.
3. I wonder if we should extend the Authorizer interface with the
Monitorable one by default or keep it
as an option for the concrete plugin to use it or not. I don't have a
strong opinion on this one. The
rationale behind this would be to keep it for advanced users. I wonder what
you and others think about this.
4. Metrics are considered as part of the public interface. It would be
great if you could move the description
in that section.
Best,
David
On Tue, May 12, 2020 at 7:46 PM Jeff Huang <je...@confluent.io> wrote:
> Hey David,
> I accepted your suggestion and add Monitorable interface. Thanks for great
> suggestion. Please review KIP-608 again and see any suggestion on name of
> function or other stuff.
>
> jeff.
>
> On 2020/05/11 15:16:30, David Jacot <dj...@confluent.io> wrote:
> > Hey,
> >
> > Thanks for the KIP.
> >
> > I think that having the possibilities to expose metrics in plugins such
> as
> > the authorizer in a
> > nice improvement. I do wonder if we could come up with a more generic way
> > to do this that
> > would apply to all plugins instead of having something specific for the
> > authorized.
> >
> > For instance, we have the `Configurable` interface that allows us to
> > configure a plugin. We
> > could perhaps think of having a `Monitorable` interface that would allow
> to
> > pass the Metrics
> > instance to the plugin. Have you thought about such an approach?
> >
> > Best,
> > David
> >
> > On Wed, May 6, 2020 at 6:43 PM Jeff Huang <je...@confluent.io>
> wrote:
> >
> > > Will update KIP.
> > > thanks!
> > >
> > > On 2020/05/06 15:09:53, Rajini Sivaram <ra...@gmail.com>
> wrote:
> > > > Hi Jeff,
> > > >
> > > > Thanks for the KIP. It looks useful since it allows authorizers to
> use
> > > the
> > > > broker's metrics instance. We could perhaps use this in
> AclAuthorizer to
> > > > generate authorizer metrics?
> > > >
> > > > Regards,
> > > >
> > > > Rajini
> > > >
> > > > On Tue, May 5, 2020 at 9:04 PM Zhiguo Huang <jeff.huang@confluent.io
> >
> > > wrote:
> > > >
> > > > >
> > > > >
> > >
> https://cwiki.apache.org/confluence/display/KAFKA/KIP-608+-+Add+a+new+method+to+AuthorizerServerInfo+Interface
> > > > >
> > > >
> > >
> >
>
Re: [DISCUSS] KIP-608: Add a new method to AuthorizerServerInfo Interface
Posted by Jeff Huang <je...@confluent.io>.
Hey David,
I accepted your suggestion and add Monitorable interface. Thanks for great suggestion. Please review KIP-608 again and see any suggestion on name of function or other stuff.
jeff.
On 2020/05/11 15:16:30, David Jacot <dj...@confluent.io> wrote:
> Hey,
>
> Thanks for the KIP.
>
> I think that having the possibilities to expose metrics in plugins such as
> the authorizer in a
> nice improvement. I do wonder if we could come up with a more generic way
> to do this that
> would apply to all plugins instead of having something specific for the
> authorized.
>
> For instance, we have the `Configurable` interface that allows us to
> configure a plugin. We
> could perhaps think of having a `Monitorable` interface that would allow to
> pass the Metrics
> instance to the plugin. Have you thought about such an approach?
>
> Best,
> David
>
> On Wed, May 6, 2020 at 6:43 PM Jeff Huang <je...@confluent.io> wrote:
>
> > Will update KIP.
> > thanks!
> >
> > On 2020/05/06 15:09:53, Rajini Sivaram <ra...@gmail.com> wrote:
> > > Hi Jeff,
> > >
> > > Thanks for the KIP. It looks useful since it allows authorizers to use
> > the
> > > broker's metrics instance. We could perhaps use this in AclAuthorizer to
> > > generate authorizer metrics?
> > >
> > > Regards,
> > >
> > > Rajini
> > >
> > > On Tue, May 5, 2020 at 9:04 PM Zhiguo Huang <je...@confluent.io>
> > wrote:
> > >
> > > >
> > > >
> > https://cwiki.apache.org/confluence/display/KAFKA/KIP-608+-+Add+a+new+method+to+AuthorizerServerInfo+Interface
> > > >
> > >
> >
>
Re: [DISCUSS] KIP-608: Add a new method to AuthorizerServerInfo Interface
Posted by Jeff Huang <je...@confluent.io>.
Hi David,
I had thought about this before. I changed mind due to we want the metrics in Authorizer in 2.6 and time restraint. But I will reconsider this. Thanks for this suggestion.
Jeff.
On 2020/05/11 15:16:30, David Jacot <dj...@confluent.io> wrote:
> Hey,
>
> Thanks for the KIP.
>
> I think that having the possibilities to expose metrics in plugins such as
> the authorizer in a
> nice improvement. I do wonder if we could come up with a more generic way
> to do this that
> would apply to all plugins instead of having something specific for the
> authorized.
>
> For instance, we have the `Configurable` interface that allows us to
> configure a plugin. We
> could perhaps think of having a `Monitorable` interface that would allow to
> pass the Metrics
> instance to the plugin. Have you thought about such an approach?
>
> Best,
> David
>
> On Wed, May 6, 2020 at 6:43 PM Jeff Huang <je...@confluent.io> wrote:
>
> > Will update KIP.
> > thanks!
> >
> > On 2020/05/06 15:09:53, Rajini Sivaram <ra...@gmail.com> wrote:
> > > Hi Jeff,
> > >
> > > Thanks for the KIP. It looks useful since it allows authorizers to use
> > the
> > > broker's metrics instance. We could perhaps use this in AclAuthorizer to
> > > generate authorizer metrics?
> > >
> > > Regards,
> > >
> > > Rajini
> > >
> > > On Tue, May 5, 2020 at 9:04 PM Zhiguo Huang <je...@confluent.io>
> > wrote:
> > >
> > > >
> > > >
> > https://cwiki.apache.org/confluence/display/KAFKA/KIP-608+-+Add+a+new+method+to+AuthorizerServerInfo+Interface
> > > >
> > >
> >
>
Re: [DISCUSS] KIP-608: Add a new method to AuthorizerServerInfo Interface
Posted by David Jacot <dj...@confluent.io>.
Hey,
Thanks for the KIP.
I think that having the possibilities to expose metrics in plugins such as
the authorizer in a
nice improvement. I do wonder if we could come up with a more generic way
to do this that
would apply to all plugins instead of having something specific for the
authorized.
For instance, we have the `Configurable` interface that allows us to
configure a plugin. We
could perhaps think of having a `Monitorable` interface that would allow to
pass the Metrics
instance to the plugin. Have you thought about such an approach?
Best,
David
On Wed, May 6, 2020 at 6:43 PM Jeff Huang <je...@confluent.io> wrote:
> Will update KIP.
> thanks!
>
> On 2020/05/06 15:09:53, Rajini Sivaram <ra...@gmail.com> wrote:
> > Hi Jeff,
> >
> > Thanks for the KIP. It looks useful since it allows authorizers to use
> the
> > broker's metrics instance. We could perhaps use this in AclAuthorizer to
> > generate authorizer metrics?
> >
> > Regards,
> >
> > Rajini
> >
> > On Tue, May 5, 2020 at 9:04 PM Zhiguo Huang <je...@confluent.io>
> wrote:
> >
> > >
> > >
> https://cwiki.apache.org/confluence/display/KAFKA/KIP-608+-+Add+a+new+method+to+AuthorizerServerInfo+Interface
> > >
> >
>
Re: [DISCUSS] KIP-608: Add a new method to AuthorizerServerInfo Interface
Posted by Jeff Huang <je...@confluent.io>.
Will update KIP.
thanks!
On 2020/05/06 15:09:53, Rajini Sivaram <ra...@gmail.com> wrote:
> Hi Jeff,
>
> Thanks for the KIP. It looks useful since it allows authorizers to use the
> broker's metrics instance. We could perhaps use this in AclAuthorizer to
> generate authorizer metrics?
>
> Regards,
>
> Rajini
>
> On Tue, May 5, 2020 at 9:04 PM Zhiguo Huang <je...@confluent.io> wrote:
>
> >
> > https://cwiki.apache.org/confluence/display/KAFKA/KIP-608+-+Add+a+new+method+to+AuthorizerServerInfo+Interface
> >
>
Re: [DISCUSS] KIP-608: Add a new method to AuthorizerServerInfo Interface
Posted by Jeff Huang <je...@confluent.io>.
1. It is intended to return broker metrics.
2. I did searching, there is no any public API return broker Metrics class so far. So this will be first time. I did find connect also expose the Metrics class, but it is not in public API.
org.apache.kafka.connect.runtime.ConnectMetrics
/**
* Get the {@link Metrics Kafka Metrics} that are managed by this object and that should be used to
* add sensors and individual metrics.
*
* @return the Kafka Metrics instance; never null
*/
public Metrics metrics() {
return metrics;
}
On 2020/05/06 15:31:07, Ismael Juma <is...@juma.me.uk> wrote:
> Thanks for the KIP. A couple of questions:
>
> 1. Is it intended for this method to return null or the broker metrics
> instance?
> 2. Is the Metrics class returned in any public APIs today or this the first
> time we are doing it?
>
> Ismael
>
> On Wed, May 6, 2020 at 8:10 AM Rajini Sivaram <ra...@gmail.com>
> wrote:
>
> > Hi Jeff,
> >
> > Thanks for the KIP. It looks useful since it allows authorizers to use the
> > broker's metrics instance. We could perhaps use this in AclAuthorizer to
> > generate authorizer metrics?
> >
> > Regards,
> >
> > Rajini
> >
> > On Tue, May 5, 2020 at 9:04 PM Zhiguo Huang <je...@confluent.io>
> > wrote:
> >
> > >
> > >
> > https://cwiki.apache.org/confluence/display/KAFKA/KIP-608+-+Add+a+new+method+to+AuthorizerServerInfo+Interface
> > >
> >
>
Re: [DISCUSS] KIP-608: Add a new method to AuthorizerServerInfo Interface
Posted by Ismael Juma <is...@juma.me.uk>.
Thanks for the KIP. A couple of questions:
1. Is it intended for this method to return null or the broker metrics
instance?
2. Is the Metrics class returned in any public APIs today or this the first
time we are doing it?
Ismael
On Wed, May 6, 2020 at 8:10 AM Rajini Sivaram <ra...@gmail.com>
wrote:
> Hi Jeff,
>
> Thanks for the KIP. It looks useful since it allows authorizers to use the
> broker's metrics instance. We could perhaps use this in AclAuthorizer to
> generate authorizer metrics?
>
> Regards,
>
> Rajini
>
> On Tue, May 5, 2020 at 9:04 PM Zhiguo Huang <je...@confluent.io>
> wrote:
>
> >
> >
> https://cwiki.apache.org/confluence/display/KAFKA/KIP-608+-+Add+a+new+method+to+AuthorizerServerInfo+Interface
> >
>
Re: [DISCUSS] KIP-608: Add a new method to AuthorizerServerInfo Interface
Posted by Rajini Sivaram <ra...@gmail.com>.
Hi Jeff,
Thanks for the KIP. It looks useful since it allows authorizers to use the
broker's metrics instance. We could perhaps use this in AclAuthorizer to
generate authorizer metrics?
Regards,
Rajini
On Tue, May 5, 2020 at 9:04 PM Zhiguo Huang <je...@confluent.io> wrote:
>
> https://cwiki.apache.org/confluence/display/KAFKA/KIP-608+-+Add+a+new+method+to+AuthorizerServerInfo+Interface
>