Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[fix][client] Fix exception when calling loadConf on a ConsumerBuilder that has a KeySharedPolicy #18345

Merged
merged 1 commit into from
Nov 16, 2022

Conversation

cbornet
Copy link
Contributor

@cbornet cbornet commented Nov 4, 2022

Motivation

The exception occurs because KeySharedPolicy can't be deserialized.

This change also adds unit tests for loadConf

Modifications

Add JsonIgnore on ConsumerBuilder's keySharedPolicy field.

Verifying this change

  • Make sure that the change passes the CI checks.

This change added tests and can be verified as follows:

  • Run ConsumerBuilderImplTest::testLoadConf and ConsumerBuilderImplTest::testLoadConfNotModified

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

If the box was checked, please highlight the changes

  • Dependencies (add or upgrade a dependency)
  • The public API
  • The schema
  • The default values of configurations
  • The threading model
  • The binary protocol
  • The REST endpoints
  • The admin CLI options
  • Anything that affects deployment

Documentation

  • doc
  • doc-required
  • doc-not-needed
  • doc-complete

Matching PR in forked repository

PR in forked repository: cbornet#6

@cbornet cbornet changed the title Fix exception when calling loadConf on a ConsumerBuilder that has a KeySharedPolicy [fix][client] Fix exception when calling loadConf on a ConsumerBuilder that has a KeySharedPolicy Nov 4, 2022
…eySharedPolicy

The exception occurs because KeySharedPolicy can't be deserialized.
This change also adds unit tests for loadConf
@codecov-commenter
Copy link

codecov-commenter commented Nov 5, 2022

Codecov Report

Merging #18345 (376f413) into master (67d9d63) will increase coverage by 6.95%.
The diff coverage is 0.00%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master   #18345      +/-   ##
============================================
+ Coverage     40.29%   47.25%   +6.95%     
- Complexity     8685    10370    +1685     
============================================
  Files           687      687              
  Lines         67441    67437       -4     
  Branches       7225     7226       +1     
============================================
+ Hits          27175    31865    +4690     
+ Misses        37257    31996    -5261     
- Partials       3009     3576     +567     
Flag Coverage Δ
unittests 47.25% <0.00%> (+6.95%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...che/bookkeeper/mledger/impl/ManagedCursorImpl.java 36.81% <0.00%> (-0.09%) ⬇️
...rg/apache/bookkeeper/mledger/impl/OpReadEntry.java 65.55% <ø> (+0.72%) ⬆️
...ar/client/impl/conf/ConsumerConfigurationData.java 92.55% <ø> (+92.55%) ⬆️
...java/org/apache/pulsar/proxy/stats/TopicStats.java 58.82% <0.00%> (-41.18%) ⬇️
.../pulsar/broker/service/SharedConsumerAssignor.java 51.85% <0.00%> (-25.93%) ⬇️
.../apache/pulsar/broker/loadbalance/LoadManager.java 61.11% <0.00%> (-16.67%) ⬇️
...tent/NonPersistentDispatcherMultipleConsumers.java 40.74% <0.00%> (-12.35%) ⬇️
...apache/pulsar/broker/service/TopicListService.java 42.62% <0.00%> (-12.30%) ⬇️
...oker/service/schema/SchemaRegistryServiceImpl.java 57.95% <0.00%> (-6.91%) ⬇️
...pulsar/broker/service/PulsarCommandSenderImpl.java 71.72% <0.00%> (-6.29%) ⬇️
... and 200 more

Copy link
Contributor

@eolivelli eolivelli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lgtm

@codelipenghui codelipenghui added this to the 2.12.0 milestone Nov 16, 2022
@codelipenghui codelipenghui merged commit 9c2ec5e into apache:master Nov 16, 2022
congbobo184 pushed a commit that referenced this pull request Nov 16, 2022
…r that has a KeySharedPolicy (#18345)

(cherry picked from commit 9c2ec5e)
@congbobo184 congbobo184 added the cherry-picked/branch-2.9 Archived: 2.9 is end of life label Nov 16, 2022
congbobo184 pushed a commit that referenced this pull request Dec 7, 2022
…r that has a KeySharedPolicy (#18345)

(cherry picked from commit 9c2ec5e)
liangyepianzhou pushed a commit that referenced this pull request Dec 14, 2022
…r that has a KeySharedPolicy (#18345)

(cherry picked from commit 9c2ec5e)
@cbornet cbornet deleted the fix-keysharedpolicy-loadconf branch December 25, 2022 20:26
nicoloboschi pushed a commit to datastax/pulsar that referenced this pull request Jan 10, 2023
…r that has a KeySharedPolicy (apache#18345)

(cherry picked from commit 9c2ec5e)
(cherry picked from commit 5e8a213)
nicoloboschi pushed a commit to datastax/pulsar that referenced this pull request Jan 11, 2023
…r that has a KeySharedPolicy (apache#18345)

(cherry picked from commit 9c2ec5e)
(cherry picked from commit 5e8a213)
Technoboy- pushed a commit that referenced this pull request Feb 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants