ojasvajain commented on code in PR #23696:
URL: https://github.com/apache/kafka/pull/23696#discussion_r4325252213
##########
server/src/main/java/org/apache/kafka/server/ClientMetricsManager.java:
##########
@@ -133,8 +133,9 @@ public Set<String> listClientMetricsResources() {
}
public void updateSubscription(String subscriptionName, Properties
properties) {
- // Validate the subscription properties.
- ClientMetricsConfigs.validate(subscriptionName, properties);
+ // Reapplying an already-committed config, not admitting a new one, so
pass properties
Review Comment:
`updateSubscription` is called in two scenarios - when a live config is
altered and propagated to the brokers, and when a broker replays the metadata
logs. For the first case, removing the validate call should be fine as we would
have run the same validation on controller during "alter".
The second case needs more thought for when a broker is restarted with a new
version. The validate call above does provide a way to check whether existing
subscriptions still follow the latest rules, since it's the same method the
controller calls during alter. But whenever we introduce a
backward-incompatible change to a subscription field in a minor version (e.g.
narrowing the interval range, or adding a mandatory field), we'd need to skip
this check for that field via some workaround and validate it with a fallback
instead. When we move to the next major version, we'd want to remove the
fallback and reinstate the validate call, so all subscriptions are required to
follow the new rules.
For our case of making the regex stricter, the fallback validation already
happens in updateClientSubscription, so this validate call adds no value here
and can be removed. In Apache Kafka 5.0, we can simply drop the fallback
without reinstating this validate call. If we ever need to make a different
backward-incompatible change to the subscription, we can revisit this then.
--
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]
