m1a2st commented on code in PR #19527:
URL: https://github.com/apache/kafka/pull/19527#discussion_r2372658102
##########
clients/src/main/java/org/apache/kafka/common/security/authenticator/LoginManager.java:
##########
@@ -53,20 +56,30 @@ public class LoginManager {
// dynamic configs (broker or client)
private static final Map<LoginMetadata<Password>, LoginManager>
DYNAMIC_INSTANCES = new HashMap<>();
- private final Login login;
+ private final Plugin<Login> loginPlugin;
private final LoginMetadata<?> loginMetadata;
private final AuthenticateCallbackHandler loginCallbackHandler;
private int refCount;
- private LoginManager(JaasContext jaasContext, String saslMechanism,
Map<String, ?> configs,
- LoginMetadata<?> loginMetadata) throws LoginException {
+ private LoginManager(
+ JaasContext jaasContext,
+ String saslMechanism,
+ Map<String, ?> configs,
+ LoginMetadata<?> loginMetadata,
+ ConnectionMode connectionMode,
+ Metrics metrics
+ ) throws LoginException {
this.loginMetadata = loginMetadata;
- this.login = Utils.newInstance(loginMetadata.loginClass);
+ Login login = Utils.newInstance(loginMetadata.loginClass);
+ if (connectionMode == ConnectionMode.SERVER)
+ this.loginPlugin = Plugin.wrapInstance(login, metrics,
SaslConfigs.SASL_LOGIN_CLASS, "mechanism", saslMechanism);
Review Comment:
> I wonder if these instances actually mess with each other's metrics? Could
they share a Selector instance?
I tested these metrics locally, and they do not interfere with each other.
Only one metric is present at a time.
| Component name | Metrics group |
mess |
|------------------------------------------|------------------------------|------|
| BrokerServer.startup() | forwarding |
X |
| BrokerServer.createShareStatePersister() | persister-channel |
X |
| TransactionMarkerChannelManager.apply() | txn-maker-channel |
X |
| RaftManager.buildNetworkClient() | raft-channel |
X |
| NetworkUtils.buildNetworkClient() | AddPartitionsManager-channel |
X |
> Have you looked at using metricGroupPrefix as a tag to differentiate them?
This seems to be set from all code
I'm not sure if using `metricGroupPrefix` as a tag to distinguish these
metrics is a good idea, since it’s not clear to users which processor or node
is setting the metric, it only indicates which component is setting it.
> We would still need to add extra tags for Login, SslEngineFactory,
AuthenticateCallbackHandler and KafkaPrincipalBuilder to make them unique
across listeners. Can you list the required tags for each of them?
| | Server
| Server client | client |
|-----------------------------|------------------------------------------------------|-----------------------------------|--------------------------|
| Login | config, class, mechanism, networkProcessor,
listener | config, class, component, node-id | config, class, client-id |
| SslEngineFactory | config, class, networkProcessor, listener
| config, class, component, node-id | config, class, client-id |
| AuthenticateCallbackHandler | config, class, mechanism, networkProcessor,
listener | config, class, component, node-id | config, class, client-id |
| KafkaPrincipalBuilder | config, class, networkProcessor, listener
| config, class, component, node-id | config, class, client-id |
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
