[GitHub] [pulsar] michaeljmarshall commented on a diff in pull request #13298: [11646][Pulsar Java client] Fix ConfigurationDataUtils loadConf strategy

[GitBox <gi...@apache.org>]

michaeljmarshall commented on code in PR #13298:
URL: https://github.com/apache/pulsar/pull/13298#discussion_r1014169543


##########
pulsar-client/src/main/java/org/apache/pulsar/client/impl/conf/ConfigurationDataUtils.java:
##########
@@ -51,20 +51,17 @@ public static ObjectMapper getThreadLocal() {
         return mapper.get();
     }
 
-    private ConfigurationDataUtils() {}
+    private ConfigurationDataUtils() {
+    }
 
     public static <T> T loadData(Map<String, Object> config,
-                                 T existingData,
-                                 Class<T> dataCls) {
+                                 T existingData) {
         ObjectMapper mapper = getThreadLocal();
         try {
-            String existingConfigJson = mapper.writeValueAsString(existingData);
-            Map<String, Object> existingConfig = mapper.readValue(existingConfigJson, Map.class);
             Map<String, Object> newConfig = new HashMap<>();
-            newConfig.putAll(existingConfig);
             newConfig.putAll(config);
             String configJson = mapper.writeValueAsString(newConfig);
-            return mapper.readValue(configJson, dataCls);
+            return mapper.readerForUpdating(existingData).readValue(configJson);

Review Comment:
   One consequence of this PR is that we will no longer have a deep copy of the configuration data. This might be fine, but it is definitely a breaking change, as some use cases might implicitly rely on that feature.
   
   An alternative solution was described here https://github.com/apache/pulsar/issues/8509 and here https://github.com/apache/pulsar/pull/8910#issuecomment-743767701.
   
   However, I don't believe that solution will help with fields that are not serializable, which is something that @cbornet was asking about.



-- 
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: commits-unsubscribe@pulsar.apache.org

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