junrao commented on a change in pull request #8826:
URL: https://github.com/apache/kafka/pull/8826#discussion_r520225611
##########
File path:
clients/src/main/java/org/apache/kafka/common/network/ChannelBuilders.java
##########
@@ -159,24 +159,25 @@ private static ChannelBuilder create(SecurityProtocol
securityProtocol,
}
// Visibility for testing
- protected static Map<String, Object> channelBuilderConfigs(final
AbstractConfig config, final ListenerName listenerName) {
- Map<String, ?> parsedConfigs;
+ @SuppressWarnings("unchecked")
+ static Map<String, Object> channelBuilderConfigs(final AbstractConfig
config, final ListenerName listenerName) {
+ Map<String, Object> parsedConfigs;
if (listenerName == null)
- parsedConfigs = config.values();
+ parsedConfigs = (Map<String, Object>) config.values();
Review comment:
Does this cover the case when listenerName is not null? I guess that can
only happen on the server side and since we don't log unused configs on the
server, so maybe this is ok for now?
##########
File path:
clients/src/main/java/org/apache/kafka/common/network/ChannelBuilders.java
##########
@@ -159,24 +159,25 @@ private static ChannelBuilder create(SecurityProtocol
securityProtocol,
}
// Visibility for testing
- protected static Map<String, Object> channelBuilderConfigs(final
AbstractConfig config, final ListenerName listenerName) {
- Map<String, ?> parsedConfigs;
+ @SuppressWarnings("unchecked")
+ static Map<String, Object> channelBuilderConfigs(final AbstractConfig
config, final ListenerName listenerName) {
+ Map<String, Object> parsedConfigs;
if (listenerName == null)
- parsedConfigs = config.values();
+ parsedConfigs = (Map<String, Object>) config.values();
else
parsedConfigs =
config.valuesWithPrefixOverride(listenerName.configPrefix());
- // include any custom configs from original configs
- Map<String, Object> configs = new HashMap<>(parsedConfigs);
config.originals().entrySet().stream()
.filter(e -> !parsedConfigs.containsKey(e.getKey())) // exclude
already parsed configs
// exclude already parsed listener prefix configs
.filter(e -> !(listenerName != null &&
e.getKey().startsWith(listenerName.configPrefix()) &&
parsedConfigs.containsKey(e.getKey().substring(listenerName.configPrefix().length()))))
// exclude keys like `{mechanism}.some.prop` if "listener.name."
prefix is present and key `some.prop` exists in parsed configs.
.filter(e -> !(listenerName != null &&
parsedConfigs.containsKey(e.getKey().substring(e.getKey().indexOf('.') + 1))))
- .forEach(e -> configs.put(e.getKey(), e.getValue()));
- return configs;
+ .forEach(e -> parsedConfigs.put(e.getKey(), e.getValue()));
+ // The callers may add new elements to return map so we should not
wrap it to a immutable map. Otherwise,
+ // the callers have to create a new map to carry more elements and
then following Get ops are not recorded.
Review comment:
(1) "so we should not wrap it to a immutable map": It's kind of weird to
have a comment on what we don't do.
(2) to return map => to returned map
##########
File path:
clients/src/main/java/org/apache/kafka/common/config/AbstractConfig.java
##########
@@ -582,6 +582,13 @@ public int hashCode() {
return originals.hashCode();
}
+ /**
+ * @return true if the input map is a recording map. otherwise, false
+ */
+ public static boolean isRecording(Map<String, ?> map) {
Review comment:
common/config/* is part of the public interface. This method seems
internal. So, could we not expose it publicly to the end user?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
