C0urante commented on code in PR #12010:
URL: https://github.com/apache/kafka/pull/12010#discussion_r853536713
##########
clients/src/main/java/org/apache/kafka/common/config/SaslConfigs.java:
##########
@@ -202,7 +202,7 @@ public static void addClientSaslSupport(ConfigDef config) {
.define(SaslConfigs.SASL_LOGIN_REFRESH_WINDOW_JITTER,
ConfigDef.Type.DOUBLE, SaslConfigs.DEFAULT_LOGIN_REFRESH_WINDOW_JITTER,
Range.between(0.0, 0.25), ConfigDef.Importance.LOW,
SaslConfigs.SASL_LOGIN_REFRESH_WINDOW_JITTER_DOC)
.define(SaslConfigs.SASL_LOGIN_REFRESH_MIN_PERIOD_SECONDS,
ConfigDef.Type.SHORT, SaslConfigs.DEFAULT_LOGIN_REFRESH_MIN_PERIOD_SECONDS,
Range.between(0, 900), ConfigDef.Importance.LOW,
SaslConfigs.SASL_LOGIN_REFRESH_MIN_PERIOD_SECONDS_DOC)
.define(SaslConfigs.SASL_LOGIN_REFRESH_BUFFER_SECONDS,
ConfigDef.Type.SHORT, SaslConfigs.DEFAULT_LOGIN_REFRESH_BUFFER_SECONDS,
Range.between(0, 3600), ConfigDef.Importance.LOW,
SaslConfigs.SASL_LOGIN_REFRESH_BUFFER_SECONDS_DOC)
- .define(SaslConfigs.SASL_MECHANISM, ConfigDef.Type.STRING,
SaslConfigs.DEFAULT_SASL_MECHANISM, ConfigDef.Importance.MEDIUM,
SaslConfigs.SASL_MECHANISM_DOC)
+ .define(SaslConfigs.SASL_MECHANISM, ConfigDef.Type.STRING,
SaslConfigs.DEFAULT_SASL_MECHANISM, ConfigDef.CompositeValidator.of(new
ConfigDef.NonNullValidator(), new ConfigDef.NonEmptyString()),
ConfigDef.Importance.MEDIUM, SaslConfigs.SASL_MECHANISM_DOC)
Review Comment:
I'm imagining a situation like this: a user has set `sasl.mechanism` to
something strange like `null` (not understanding that this isn't necessary),
they haven't configured their consumer/producer to use SASL, then they upgrade
to a version of Kafka that includes this change, and things unexpectedly start
breaking for them.
I understand that most people don't put garbage in their configs, but I also
don't think it's worth the tradeoff here of causing configs that used to be
valid to suddenly start failing.
What I believe is more warranted here is context-dependent validation that
uses the same core logic that you propose here (i.e., disallowing null and
empty values), but only does it if SASL is enabled for the consumer/producer.
And, if SASL is disabled and the user has provided a value for this property
anyways, it's probably worth emitting a warning letting them know that this
value is not being used by the consumer/producer.
--
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: jira-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
