C0urante commented on code in PR #13163:
URL: https://github.com/apache/kafka/pull/13163#discussion_r1091016715
##########
clients/src/main/java/org/apache/kafka/common/config/AbstractConfig.java:
##########
@@ -246,13 +246,22 @@ public Map<String, Object> originals(Map<String, Object>
configOverrides) {
*/
public Map<String, String> originalsStrings() {
Map<String, String> copy = new RecordingMap<>();
+ copyAsStrings(originals, copy);
+ return copy;
+ }
+
+ /**
+ * Ensures that all values of a map are strings, and copies them to
another map.
+ * @param originals The map to validate.
+ * @param copy The target to copy to.
+ */
+ protected static void copyAsStrings(Map<String, ?> originals, Map<String,
String> copy) {
Review Comment:
This counts as a change to public interface since subclasses of
`AbstractConfig` would be able to access this new method. And since this wasn't
mentioned in the KIP, we probably shouldn't do that here.
##########
connect/mirror/src/main/java/org/apache/kafka/connect/mirror/MirrorMakerConfig.java:
##########
@@ -82,10 +82,16 @@ public class MirrorMakerConfig extends AbstractConfig {
static final String TARGET_PREFIX = "target.";
private final Plugins plugins;
-
+
+ private final Map<String, String> rawProperties;
+
+ @SuppressWarnings("unchecked")
public MirrorMakerConfig(Map<?, ?> props) {
super(CONFIG_DEF, props, true);
plugins = new Plugins(originalsStrings());
+
+ rawProperties = new HashMap<>();
+ copyAsStrings((Map<String, ?>) props, rawProperties);
Review Comment:
It looks like this is intended to get around the fact that the constructor
accepts a `Map<?, ?>` but we really want to work with a `Map<String, String>`.
Could we change the constructor to accept a `Map<String, String>` and store
that as our `rawProperties` field, instead of introducing the `copyAsStrings`
method?
##########
connect/mirror/src/main/java/org/apache/kafka/connect/mirror/MirrorMakerConfig.java:
##########
@@ -130,15 +136,14 @@ public List<SourceAndTarget> clusterPairs() {
*/
public MirrorClientConfig clientConfig(String cluster) {
Map<String, String> props = new HashMap<>();
- props.putAll(originalsStrings());
+ props.putAll(rawProperties);
Review Comment:
Why change this part? Aren't we transforming the configs later at line 141
anyways?
--
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
