Hi Jeff, Thanks for the updated KIP, looks good.
Regards, Rajini On Fri, May 15, 2020 at 5:57 PM Jeff Huang <jeff.hu...@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 <dja...@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 <jeff.hu...@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 <dja...@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 <jeff.hu...@confluent.io> > > > wrote: > > > > > > > > > Will update KIP. > > > > > thanks! > > > > > > > > > > On 2020/05/06 15:09:53, Rajini Sivaram <rajinisiva...@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.hu...@confluent.io > > > > > > > > > wrote: > > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://cwiki.apache.org/confluence/display/KAFKA/KIP-608+-+Add+a+new+method+to+AuthorizerServerInfo+Interface > > > > > > > > > > > > > > > > > > > > > > > > > > > >
