chia7712 commented on code in PR #23696:
URL: https://github.com/apache/kafka/pull/23696#discussion_r4331066438
##########
server/src/main/java/org/apache/kafka/server/metrics/ClientMetricsConfigs.java:
##########
@@ -185,36 +230,74 @@ private static void validateConfigs(Map<?, ?> configs) {
* NOTES:
* Client match pattern splits the input into two parts separated by first
occurrence of the character '='
*
+ * @param subscriptionName Name of the client metrics subscription these
patterns belong to
* @param patterns List of client matching pattern strings
* @return map of client matching pattern entries
*/
- public static Map<String, Pattern> parseMatchingPatterns(List<String>
patterns) {
+ public static Map<String, ClientMatchPattern> parseMatchingPatterns(String
subscriptionName, List<String> patterns) {
if (patterns == null || patterns.isEmpty()) {
return Map.of();
}
- Map<String, Pattern> patternsMap = new HashMap<>();
+ Map<String, ClientMatchPattern> patternsMap = new HashMap<>();
patterns.forEach(pattern -> {
- // The pattern value may contain '=' characters, so split only at
the first occurrence.
- String[] nameValuePair = pattern.split("=", 2);
- if (nameValuePair.length != 2) {
- throw new InvalidConfigurationException("Illegal client
matching pattern: " + pattern);
- }
+ String[] nameValuePair = splitMatchPattern(pattern);
+ String param = nameValuePair[0];
+ String patternValue = nameValuePair[1];
- String param = nameValuePair[0].trim();
- if (!isValidParam(param)) {
- throw new InvalidConfigurationException("Illegal client
matching pattern: " + pattern);
- }
+ patternsMap.put(param, compileLenient(subscriptionName, param,
patternValue));
+ });
+ return patternsMap;
+ }
+
+ /**
+ * Compiles a single match pattern with RE2/J, falling back to
java.util.regex (and logging a
+ * warning) if that fails.
+ *
+ * See also {@link #compileStrict(String, String)}.
+ *
+ * @throws InvalidConfigurationException if the pattern is not valid under
either engine
+ */
+ @SuppressWarnings("removal")
+ private static ClientMatchPattern compileLenient(String subscriptionName,
String param, String patternValue) {
+ try {
+ return ClientMatchPattern.ofRe2(Pattern.compile(patternValue));
+ } catch (PatternSyntaxException re2Exception) {
try {
- Pattern patternValue =
Pattern.compile(nameValuePair[1].trim());
- patternsMap.put(param, patternValue);
- } catch (PatternSyntaxException e) {
- throw new InvalidConfigurationException("Illegal client
matching pattern: " + pattern);
+ java.util.regex.Pattern legacyPattern =
java.util.regex.Pattern.compile(patternValue);
+ LOG.warn("Client metrics subscription '{}' match pattern
'{}={}' relies on deprecated " +
+ "java.util.regex syntax ({}) and will stop being
supported in Apache Kafka 5.0. " +
+ "Please update it to valid RE2/J syntax.",
+ subscriptionName, param, patternValue,
re2Exception.getDescription());
+ return ClientMatchPattern.ofLegacy(legacyPattern);
+ } catch (java.util.regex.PatternSyntaxException legacyException) {
+ throw new InvalidConfigurationException("Illegal client
matching pattern: " + param + "=" + patternValue);
Review Comment:
Should we add `legacyException` as cause?
##########
server/src/main/java/org/apache/kafka/server/metrics/ClientMetricsConfigs.java:
##########
@@ -185,36 +230,74 @@ private static void validateConfigs(Map<?, ?> configs) {
* NOTES:
* Client match pattern splits the input into two parts separated by first
occurrence of the character '='
*
+ * @param subscriptionName Name of the client metrics subscription these
patterns belong to
* @param patterns List of client matching pattern strings
* @return map of client matching pattern entries
*/
- public static Map<String, Pattern> parseMatchingPatterns(List<String>
patterns) {
+ public static Map<String, ClientMatchPattern> parseMatchingPatterns(String
subscriptionName, List<String> patterns) {
if (patterns == null || patterns.isEmpty()) {
return Map.of();
}
- Map<String, Pattern> patternsMap = new HashMap<>();
+ Map<String, ClientMatchPattern> patternsMap = new HashMap<>();
patterns.forEach(pattern -> {
- // The pattern value may contain '=' characters, so split only at
the first occurrence.
- String[] nameValuePair = pattern.split("=", 2);
- if (nameValuePair.length != 2) {
- throw new InvalidConfigurationException("Illegal client
matching pattern: " + pattern);
- }
+ String[] nameValuePair = splitMatchPattern(pattern);
+ String param = nameValuePair[0];
+ String patternValue = nameValuePair[1];
- String param = nameValuePair[0].trim();
- if (!isValidParam(param)) {
- throw new InvalidConfigurationException("Illegal client
matching pattern: " + pattern);
- }
+ patternsMap.put(param, compileLenient(subscriptionName, param,
patternValue));
+ });
+ return patternsMap;
+ }
+
+ /**
+ * Compiles a single match pattern with RE2/J, falling back to
java.util.regex (and logging a
+ * warning) if that fails.
+ *
+ * See also {@link #compileStrict(String, String)}.
+ *
+ * @throws InvalidConfigurationException if the pattern is not valid under
either engine
+ */
+ @SuppressWarnings("removal")
+ private static ClientMatchPattern compileLenient(String subscriptionName,
String param, String patternValue) {
+ try {
+ return ClientMatchPattern.ofRe2(Pattern.compile(patternValue));
+ } catch (PatternSyntaxException re2Exception) {
try {
- Pattern patternValue =
Pattern.compile(nameValuePair[1].trim());
- patternsMap.put(param, patternValue);
- } catch (PatternSyntaxException e) {
- throw new InvalidConfigurationException("Illegal client
matching pattern: " + pattern);
+ java.util.regex.Pattern legacyPattern =
java.util.regex.Pattern.compile(patternValue);
+ LOG.warn("Client metrics subscription '{}' match pattern
'{}={}' relies on deprecated " +
+ "java.util.regex syntax ({}) and will stop being
supported in Apache Kafka 5.0. " +
+ "Please update it to valid RE2/J syntax.",
+ subscriptionName, param, patternValue,
re2Exception.getDescription());
+ return ClientMatchPattern.ofLegacy(legacyPattern);
Review Comment:
Please open a blocker ticket for 5.0.0 for removing this fallback, and
reference it in the comment :)
##########
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:
yes, I prefer removing the validation from this path. BTW, I guess removing
the check will break some tests
--
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]
