[GitHub] [kafka] C0urante commented on a diff in pull request #13163: KAFKA-14653: MirrorMakerConfig using raw properties instead of post-r…

["C0urante (via GitHub)" <gi...@apache.org>]

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-unsubscribe@kafka.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org