You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pulsar.apache.org by GitBox <gi...@apache.org> on 2022/12/06 10:20:45 UTC
[GitHub] [pulsar] mgrenonville opened a new pull request, #13298: [11646][Pulsar Java client] Fix ConfigurationDataUtils loadConf strategy
mgrenonville opened a new pull request, #13298:
URL: https://github.com/apache/pulsar/pull/13298
Fixes https://github.com/apache/pulsar/issues/11646
### Motivation
When using `loadConf`, ConfigurationDataUtils was loading existing values
in a Map using Jackson mapper. If there is a `@JsonIgnore`, Jackson
ignores theses fields, but there will be forgotten in the new instance.
### Modifications
Using `readerForUpdating(existingData).readValue(configJson)` will
refresh only overriden values in config map.
### Verifying this change
- [x] Make sure that the change passes the CI checks.
This change is already covered by existing tests, such as *org.apache.pulsar.client.impl.conf.ConfigurationDataUtilsTest*.
### Does this pull request potentially affect one of the following parts:
*If `yes` was chosen, please highlight the changes*
- Dependencies (does it add or upgrade a dependency): no
- The public API: no
- The schema: no
- The default values of configurations: no
- The wire protocol: no
- The rest endpoints: no
- The admin cli options: no
- Anything that affects deployment: no (at least, it will no longer fails silently on `@JsonIgnore` properties)
### Documentation
Check the box below and label this PR (if you have committer privilege).
Need to update docs?
- [x] `no-need-doc`
This was a bug in `loadConf` code, it is intended to work as follow (I think)
- [x] `doc-not-needed`
--
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
[GitHub] [pulsar] michaeljmarshall commented on pull request #13298: [11646][Pulsar Java client] Fix ConfigurationDataUtils loadConf strategy
Posted by GitBox <gi...@apache.org>.
michaeljmarshall commented on PR #13298:
URL: https://github.com/apache/pulsar/pull/13298#issuecomment-1306706422
We can add a new method and deprecate the other. A deprecation would make it very clear that the semantics changed and we could document the change.
--
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
[GitHub] [pulsar] eolivelli commented on pull request #13298: [11646][Pulsar Java client] Fix ConfigurationDataUtils loadConf strategy
Posted by GitBox <gi...@apache.org>.
eolivelli commented on PR #13298:
URL: https://github.com/apache/pulsar/pull/13298#issuecomment-1303567569
@mgrenonville would you have to address the comments ?
This patch is really valuable
--
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
[GitHub] [pulsar] eolivelli commented on a diff in pull request #13298: [11646][Pulsar Java client] Fix ConfigurationDataUtils loadConf strategy
Posted by GitBox <gi...@apache.org>.
eolivelli commented on code in PR #13298:
URL: https://github.com/apache/pulsar/pull/13298#discussion_r1014040703
##########
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) {
Review Comment:
yes, I think it could have been used by users.
we should keep the old, make it work the same way as before and use the new method
--
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
[GitHub] [pulsar] congbobo184 commented on pull request #13298: [11646][Pulsar Java client] Fix ConfigurationDataUtils loadConf strategy
Posted by GitBox <gi...@apache.org>.
congbobo184 commented on PR #13298:
URL: https://github.com/apache/pulsar/pull/13298#issuecomment-1318533014
@mgrenonville hi, I move this PR to release/2.9.5, if you have any questions, please ping me. thanks.
--
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
[GitHub] [pulsar] cbornet commented on pull request #13298: [11646][Pulsar Java client] Fix ConfigurationDataUtils loadConf strategy
Posted by GitBox <gi...@apache.org>.
cbornet commented on PR #13298:
URL: https://github.com/apache/pulsar/pull/13298#issuecomment-1303241520
Should we allow `loadConf` to set non-serializable fields ?
It's possible to do it by saving the non-serializable field in a var before calling `ConfigurationDataUtils::loadData` and apply it after. I'll do a PR for discussion.
--
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
[GitHub] [pulsar] Jason918 commented on pull request #13298: [11646][Pulsar Java client] Fix ConfigurationDataUtils loadConf strategy
Posted by GitBox <gi...@apache.org>.
Jason918 commented on PR #13298:
URL: https://github.com/apache/pulsar/pull/13298#issuecomment-1229147288
@mgrenonville ping
--
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
[GitHub] [pulsar] poorbarcode commented on pull request #13298: [fix][client] Fix ConfigurationDataUtils loadConf strategy
Posted by "poorbarcode (via GitHub)" <gi...@apache.org>.
poorbarcode commented on PR #13298:
URL: https://github.com/apache/pulsar/pull/13298#issuecomment-1501567586
Since we will start the RC version of `3.0.0` on `2023-04-11`, I will change the label/milestone of PR who have not been merged.
- The PR of type `feature` is deferred to `3.1.0`
- The PR of type `fix` is deferred to `3.0.1`
So drag this PR to `3.0.1`
--
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
[GitHub] [pulsar] michaeljmarshall commented on pull request #13298: [fix][client] Fix ConfigurationDataUtils loadConf strategy
Posted by "michaeljmarshall (via GitHub)" <gi...@apache.org>.
michaeljmarshall commented on PR #13298:
URL: https://github.com/apache/pulsar/pull/13298#issuecomment-1610272240
As discussed on the mailing list https://lists.apache.org/thread/w4jzk27qhtosgsz7l9bmhf1t7o9mxjhp, there is no plan to release 2.9.6, so I am going to remove the release/2.9.6 label
--
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
[GitHub] [pulsar] tisonkun commented on pull request #13298: [11646][Pulsar Java client] Fix ConfigurationDataUtils loadConf strategy
Posted by GitBox <gi...@apache.org>.
tisonkun commented on PR #13298:
URL: https://github.com/apache/pulsar/pull/13298#issuecomment-1339083985
https://github.com/apache/pulsar/issues/11646#issuecomment-1303738507
--
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
[GitHub] [pulsar] tisonkun closed pull request #13298: [11646][Pulsar Java client] Fix ConfigurationDataUtils loadConf strategy
Posted by GitBox <gi...@apache.org>.
tisonkun closed pull request #13298: [11646][Pulsar Java client] Fix ConfigurationDataUtils loadConf strategy
URL: https://github.com/apache/pulsar/pull/13298
--
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
[GitHub] [pulsar] BewareMyPower commented on pull request #13298: [11646][Pulsar Java client] Fix ConfigurationDataUtils loadConf strategy
Posted by GitBox <gi...@apache.org>.
BewareMyPower commented on PR #13298:
URL: https://github.com/apache/pulsar/pull/13298#issuecomment-1198411619
Could you continue this work?
--
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
[GitHub] [pulsar] cbornet commented on a diff in pull request #13298: [11646][Pulsar Java client] Fix ConfigurationDataUtils loadConf strategy
Posted by GitBox <gi...@apache.org>.
cbornet commented on code in PR #13298:
URL: https://github.com/apache/pulsar/pull/13298#discussion_r1013850542
##########
pulsar-client/src/main/java/org/apache/pulsar/client/impl/conf/ConsumerConfigurationData.java:
##########
@@ -134,6 +131,7 @@ public int getMaxPendingChuckedMessage() {
private RegexSubscriptionMode regexSubscriptionMode = RegexSubscriptionMode.PersistentOnly;
+ @JsonIgnore
Review Comment:
I think it's not needed anymore since https://github.com/apache/pulsar/issues/16487
--
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
[GitHub] [pulsar] cbornet commented on a diff in pull request #13298: [11646][Pulsar Java client] Fix ConfigurationDataUtils loadConf strategy
Posted by GitBox <gi...@apache.org>.
cbornet commented on code in PR #13298:
URL: https://github.com/apache/pulsar/pull/13298#discussion_r1013900059
##########
pulsar-client/src/main/java/org/apache/pulsar/client/impl/conf/ConsumerConfigurationData.java:
##########
@@ -149,12 +147,13 @@ public int getMaxPendingChuckedMessage() {
private boolean resetIncludeHead = false;
+ @JsonIgnore
Review Comment:
KeySharedPolicy is not deserializable:
```java
@Test
public void test() {
Map<String, Object> config = new HashMap<>();
config.put("keySharedPolicy", KeySharedPolicy.autoSplitHashRange());
consumerBuilderImpl.loadConf(config);
}
```
fails with `com.fasterxml.jackson.databind.exc.InvalidDefinitionException: Cannot construct instance of org.apache.pulsar.client.api.KeySharedPolicy`
--
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
[GitHub] [pulsar] eolivelli commented on pull request #13298: [11646][Pulsar Java client] Fix ConfigurationDataUtils loadConf strategy
Posted by GitBox <gi...@apache.org>.
eolivelli commented on PR #13298:
URL: https://github.com/apache/pulsar/pull/13298#issuecomment-1303566028
@Jason918 we should add this in 2.11 before the release
--
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
[GitHub] [pulsar] michaeljmarshall commented on a diff in pull request #13298: [11646][Pulsar Java client] Fix ConfigurationDataUtils loadConf strategy
Posted by 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
[GitHub] [pulsar] cbornet commented on pull request #13298: [11646][Pulsar Java client] Fix ConfigurationDataUtils loadConf strategy
Posted by GitBox <gi...@apache.org>.
cbornet commented on PR #13298:
URL: https://github.com/apache/pulsar/pull/13298#issuecomment-1303237568
Although I think the DeadLetterPolicy crash is solved by https://github.com/apache/pulsar/issues/16487, we still have the issue that `loadConf` resets non-serializable fields to `null`. This PR prevents that.
Also it should be documented that `loadConf` only loads fields that are serializable. For instance, `loadConf(Map.of("messageListener", messageListener))` does nothing.
--
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
[GitHub] [pulsar] cbornet commented on pull request #13298: [11646][Pulsar Java client] Fix ConfigurationDataUtils loadConf strategy
Posted by GitBox <gi...@apache.org>.
cbornet commented on PR #13298:
URL: https://github.com/apache/pulsar/pull/13298#issuecomment-1304458025
@michaeljmarshall We could add `loadConfig` and deprecate `loadConf`. WDYT ?
--
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
[GitHub] [pulsar] michaeljmarshall commented on pull request #13298: [11646][Pulsar Java client] Fix ConfigurationDataUtils loadConf strategy
Posted by GitBox <gi...@apache.org>.
michaeljmarshall commented on PR #13298:
URL: https://github.com/apache/pulsar/pull/13298#issuecomment-1303769334
@cbornet - is it possible to add a new method that does not require serializability? That would let us avoid a change in the implicit method contract.
--
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
[GitHub] [pulsar] cbornet commented on pull request #13298: [11646][Pulsar Java client] Fix ConfigurationDataUtils loadConf strategy
Posted by GitBox <gi...@apache.org>.
cbornet commented on PR #13298:
URL: https://github.com/apache/pulsar/pull/13298#issuecomment-1339095625
I don't think #11646 is related to this
--
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
[GitHub] [pulsar] github-actions[bot] commented on pull request #13298: [11646][Pulsar Java client] Fix ConfigurationDataUtils loadConf strategy
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #13298:
URL: https://github.com/apache/pulsar/pull/13298#issuecomment-1140611542
The pr had no activity for 30 days, mark with Stale label.
--
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