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