[fix][client] Fix ConfigurationDataUtils loadConf strategy

[mgrenonville]

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

[mgrenonville]

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]

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

[gaoran10]

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

[github-actions[bot]]

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

[github-actions[bot]]

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

[BewareMyPower]

Could you continue this work?

[Jason918]

@mgrenonville ping

[cbornet]

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.

[cbornet]

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.

[eolivelli]

@Jason918 we should add this in 2.11 before the release

[eolivelli]

@mgrenonville would you have to address the comments ? This patch is really valuable

[michaeljmarshall]

@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.

[cbornet]

@michaeljmarshall We could add loadConfig and deprecate loadConf. WDYT ?

[michaeljmarshall]

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.

[congbobo184]

@mgrenonville hi, I move this PR to release/2.9.5, if you have any questions, please ping me. thanks.

[tisonkun]

https://github.com/apache/pulsar/issues/11646#issuecomment-1303738507

[cbornet]

I don't think #11646 is related to this

[poorbarcode]

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]

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