mimaison commented on code in PR #19527:
URL: https://github.com/apache/kafka/pull/19527#discussion_r2368711351
##########
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:
Sorry again for the delay. Trying to catalog the `Selector` instances we
create server-side, I see:
Without any tags:
- BrokerServer.startup()
- BrokerServer.createShareStatePersister()
- TransactionMarkerChannelManager.apply()
- RaftManager.buildNetworkClient()
- NetworkUtils.buildNetworkClient()
I wonder if these instances actually mess with each other's metrics? Could
they share a Selector instance?
With some tags:
- SockerServer.createSelector(): "listener", "networkProcessor"
- BrokerBlockingSender initialization: "broker-id", "fetcher-id"
- NodeToControllerChannelManagerImpl.newRequestThread(): "BrokerId"
Note the inconsistency in tag naming, we have a mix of kebab-case and
camel-case. I think overall we use kebab-case for most metrics.
If we add tags to all Selector instances, this will create quite a lot of
new metrics. This would definitively need a separate KIP.
Have you looked at using metricGroupPrefix as a tag to differentiate them?
This seems to be set from all code paths and to different values. 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?
--
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]
