[fix][client] Fix ConfigurationDataUtils loadConf strategy #13298

mgrenonville · 2021-12-14T14:37:29Z

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

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?

no-need-doc
This was a bug in loadConf code, it is intended to work as follow (I think)
doc-not-needed

codelipenghui · 2021-12-15T07:40:41Z

pulsar-client/src/main/java/org/apache/pulsar/client/impl/conf/ConfigurationDataUtils.java

@@ -51,20 +51,18 @@ 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) {


The dataCls becomes useless after the new change, we should remove it.

Thanks for review !
You're right, I've done the update.

Fixes apache#11646 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. Using `readerForUpdating(existingData).readValue(configJson)` will refresh only overriden values in config map. Since it's using existing object to write values, `dataCls` param becomes useless, thus removed.

mgrenonville · 2022-01-05T20:43:38Z

Hi @codelipenghui !
It's strange, I see that CI test are failing however, when executing these tests on my computer, they work. Are they known to be flaky ?

codelipenghui · 2022-01-07T05:09:58Z

@mgrenonville You can left a comment with content /pulsarbot run-failure-checks to trigger to rerun the failed tests.

gaoran10 · 2022-01-09T16:57:33Z

Maybe we could add a construct method for DeadLetterPolicy, then we could use the conf to set DeadLetterPolicy.

codelipenghui · 2022-01-18T05:51:51Z

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


Will this introduce breaking change?

I think it's not needed anymore since #16487

michaeljmarshall

I am requesting changes because I think this is a breaking change. If that is incorrect, please let me know.

michaeljmarshall · 2022-02-11T21:58:26Z

pulsar-client/src/main/java/org/apache/pulsar/client/impl/conf/ConfigurationDataUtils.java

    public static <T> T loadData(Map<String, Object> config,
-                                 T existingData,
-                                 Class<T> dataCls) {
+                                 T existingData) {


Unless I'm mistaken, I believe this is part of the public API and could be used by user applications to build the ClientConfigurationData. We would need to find a way to introduce this without breaking the API.

+1. We can keep the old method and just mark deprecated.

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

Jason918 · 2022-02-12T03:38:50Z

pulsar-client/src/main/java/org/apache/pulsar/client/impl/conf/ConfigurationDataUtils.java

    public static <T> T loadData(Map<String, Object> config,
-                                 T existingData,
-                                 Class<T> dataCls) {
+                                 T existingData) {


+1. We can keep the old method and just mark deprecated.

Jason918 · 2022-02-12T03:41:05Z

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


Can you add some details about why we add @JsonIgnore here?

KeySharedPolicy is not deserializable:

@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

github-actions · 2022-03-19T01:56:40Z

The pr had no activity for 30 days, mark with Stale label.

github-actions · 2022-05-30T02:15:26Z

The pr had no activity for 30 days, mark with Stale label.

tisonkun · 2022-12-06T10:12:49Z

#11646 (comment)

cbornet · 2022-12-06T10:20:42Z

I don't think #11646 is related to this

poorbarcode · 2023-04-10T08:50:25Z

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

michaeljmarshall · 2023-06-27T21:56:34Z

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

github-actions bot assigned mgrenonville Dec 14, 2021

github-actions bot added the doc-not-needed Your PR changes do not impact docs label Dec 14, 2021

codelipenghui reviewed Dec 15, 2021

View reviewed changes

codelipenghui added this to the 2.10.0 milestone Dec 15, 2021

codelipenghui added release/2.8.3 release/2.9.2 labels Dec 15, 2021

mgrenonville force-pushed the fix_load_conf branch from 68ff2b1 to 6a6b30e Compare December 16, 2021 09:26

mgrenonville requested a review from codelipenghui December 20, 2021 14:39

codelipenghui approved these changes Dec 23, 2021

View reviewed changes

codelipenghui modified the milestones: 2.10.0, 2.11.0 Jan 18, 2022

codelipenghui added release/2.9.3 and removed release/2.9.2 labels Jan 18, 2022

codelipenghui reviewed Jan 18, 2022

View reviewed changes

michaeljmarshall requested changes Feb 11, 2022

View reviewed changes

michaeljmarshall added release/2.8.4 and removed release/2.8.3 labels Feb 11, 2022

Jason918 reviewed Feb 12, 2022

View reviewed changes

codelipenghui added the release/2.10.1 label Feb 16, 2022

github-actions bot added the lifecycle/stale label Mar 19, 2022

codelipenghui added release/2.10.2 and removed release/2.10.1 labels Jun 9, 2022

mattisonchao added release/2.9.4 and removed release/2.9.3 labels Jun 9, 2022

tisonkun closed this Dec 6, 2022

cbornet reopened this Dec 6, 2022

tisonkun added release/2.11.1 and removed release/2.11.0 labels Dec 6, 2022

tisonkun changed the title ~~[11646][Pulsar Java client] Fix ConfigurationDataUtils loadConf strategy~~ [fix][client] Fix ConfigurationDataUtils loadConf strategy Dec 10, 2022

liangyepianzhou added release/2.10.4 and removed release/2.10.3 labels Feb 1, 2023

coderzc added release/2.9.6 and removed release/2.9.5 labels Mar 3, 2023

poorbarcode modified the milestones: 3.0.0, 3.1.0 Apr 10, 2023

poorbarcode added release/3.0.1 and removed release/2.8.5 labels Apr 10, 2023

RobertIndie added release/3.0.1 release/3.0.2 and removed release/3.0.1 labels Jun 8, 2023

michaeljmarshall removed the release/2.9.6 label Jun 27, 2023

Technoboy- modified the milestones: 3.1.0, 3.2.0 Jul 31, 2023

poorbarcode added release/3.0.3 and removed release/3.0.2 labels Oct 24, 2023

Technoboy- modified the milestones: 3.2.0, 3.3.0 Dec 22, 2023

coderzc modified the milestones: 3.3.0, 3.4.0 May 8, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[fix][client] Fix ConfigurationDataUtils loadConf strategy #13298

[fix][client] Fix ConfigurationDataUtils loadConf strategy #13298

mgrenonville commented Dec 14, 2021 •

edited by tisonkun

codelipenghui Dec 15, 2021

mgrenonville Dec 16, 2021

mgrenonville commented Jan 5, 2022 •

edited

codelipenghui commented Jan 7, 2022

gaoran10 commented Jan 9, 2022 •

edited

codelipenghui Jan 18, 2022

cbornet Nov 4, 2022

michaeljmarshall left a comment

michaeljmarshall Feb 11, 2022

Jason918 Feb 12, 2022

eolivelli Nov 4, 2022

Jason918 Feb 12, 2022

Jason918 Feb 12, 2022

cbornet Nov 4, 2022

github-actions bot commented Mar 19, 2022

github-actions bot commented May 30, 2022

tisonkun commented Dec 6, 2022

cbornet commented Dec 6, 2022

poorbarcode commented Apr 10, 2023

michaeljmarshall commented Jun 27, 2023

		@@ -134,6 +131,7 @@ public int getMaxPendingChuckedMessage() {

		private RegexSubscriptionMode regexSubscriptionMode = RegexSubscriptionMode.PersistentOnly;

		@JsonIgnore

		@@ -149,12 +147,13 @@ public int getMaxPendingChuckedMessage() {

		private boolean resetIncludeHead = false;

		@JsonIgnore

[fix][client] Fix ConfigurationDataUtils loadConf strategy #13298

Are you sure you want to change the base?

[fix][client] Fix ConfigurationDataUtils loadConf strategy #13298

Conversation

mgrenonville commented Dec 14, 2021 • edited by tisonkun

Motivation

Modifications

Verifying this change

Does this pull request potentially affect one of the following parts:

Documentation

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mgrenonville commented Jan 5, 2022 • edited

codelipenghui commented Jan 7, 2022

gaoran10 commented Jan 9, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michaeljmarshall left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Mar 19, 2022

github-actions bot commented May 30, 2022

tisonkun commented Dec 6, 2022

cbornet commented Dec 6, 2022

poorbarcode commented Apr 10, 2023

michaeljmarshall commented Jun 27, 2023

mgrenonville commented Dec 14, 2021 •

edited by tisonkun

mgrenonville commented Jan 5, 2022 •

edited

gaoran10 commented Jan 9, 2022 •

edited