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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
