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]Fixed getting an incorrect maxMessageSize value when accessing multiple clusters in the same process #22306

Merged
merged 5 commits into from
Mar 21, 2024

Conversation

chenhongSZ
Copy link
Contributor

@chenhongSZ chenhongSZ commented Mar 19, 2024

Motivation

We are using two PulsarClients within the same process to connect to cluster A (maxMessageSize=5M) and cluster B (maxMessageSize=16M), and sometimes the following error occurs.

Got corrupted payload message size 5251327 at org.apache.pulsar.common.api.proto.MessageIdData@5e940463

I think the root cause is that the client's org.apache.pulsar.client.impl.ClientCnx#maxMessageSize variable is static, which does not support different configurations of two clusters at the same time.

Modifications

  1. change the org.apache.pulsar.client.impl.ClientCnx#maxMessageSize variable to be non-static, share this variable only within the same connection.

Documentation

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

Matching PR in forked repository

PR in forked repository:

Copy link

@chenhongSZ Please add the following content to your PR description and select a checkbox:

- [ ] `doc` <!-- Your PR contains doc changes -->
- [ ] `doc-required` <!-- Your PR changes impact docs and you will update later -->
- [ ] `doc-not-needed` <!-- Your PR changes do not impact docs -->
- [ ] `doc-complete` <!-- Docs have been already added -->

@github-actions github-actions bot added doc-not-needed Your PR changes do not impact docs and removed doc-label-missing labels Mar 19, 2024
@lhotari lhotari added this to the 3.3.0 milestone Mar 19, 2024
Copy link
Member

@lhotari lhotari left a comment

Choose a reason for hiding this comment

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

LGTM, good work!

@lhotari
Copy link
Member

lhotari commented Mar 19, 2024

Please fix checkstyle error src/test/java/org/apache/pulsar/client/api/SimpleProducerConsumerTest.java:[96,8] (imports) UnusedImports: Unused import: org.apache.pulsar.client.impl.ClientCnx.

@chenhongSZ
Copy link
Contributor Author

Please fix checkstyle error src/test/java/org/apache/pulsar/client/api/SimpleProducerConsumerTest.java:[96,8] (imports) UnusedImports: Unused import: org.apache.pulsar.client.impl.ClientCnx.

fixed

@lhotari
Copy link
Member

lhotari commented Mar 20, 2024

It might be necessary to fix this test:
org.apache.pulsar.broker.service.MaxMessageSizeTest.testChunkingMaxMessageSize Time elapsed: 2.629 s <<< FAILURE!

@chenhongSZ
Copy link
Contributor Author

It might be necessary to fix this test: org.apache.pulsar.broker.service.MaxMessageSizeTest.testChunkingMaxMessageSize Time elapsed: 2.629 s <<< FAILURE!

oh, I'll fix it later

@chenhongSZ
Copy link
Contributor Author

/pulsarbot run-failure-checks

@dao-jun dao-jun closed this Mar 21, 2024
@dao-jun dao-jun reopened this Mar 21, 2024
@codecov-commenter
Copy link

codecov-commenter commented Mar 21, 2024

Codecov Report

Attention: Patch coverage is 82.14286% with 5 lines in your changes are missing coverage. Please review.

Project coverage is 73.81%. Comparing base (bbc6224) to head (a5c7d93).
Report is 70 commits behind head on master.

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff              @@
##             master   #22306      +/-   ##
============================================
+ Coverage     73.57%   73.81%   +0.23%     
- Complexity    32624    32841     +217     
============================================
  Files          1877     1887      +10     
  Lines        139502   139479      -23     
  Branches      15299    15292       -7     
============================================
+ Hits         102638   102955     +317     
+ Misses        28908    28600     -308     
+ Partials       7956     7924      -32     
Flag Coverage Δ
inttests 26.94% <46.42%> (+2.36%) ⬆️
systests 24.63% <42.85%> (+0.30%) ⬆️
unittests 73.04% <82.14%> (+0.20%) ⬆️

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

Files Coverage Δ
.../java/org/apache/pulsar/client/impl/ClientCnx.java 71.91% <ø> (+0.13%) ⬆️
...g/apache/pulsar/client/impl/ConnectionHandler.java 86.99% <100.00%> (+0.21%) ⬆️
...va/org/apache/pulsar/client/impl/ProducerImpl.java 84.05% <100.00%> (+0.46%) ⬆️
...va/org/apache/pulsar/client/impl/ConsumerImpl.java 77.83% <50.00%> (+0.26%) ⬆️
...sar/client/impl/AbstractBatchMessageContainer.java 80.43% <50.00%> (+0.88%) ⬆️
.../pulsar/client/impl/BatchMessageContainerImpl.java 80.89% <60.00%> (ø)

... and 113 files with indirect coverage changes

@chenhongSZ
Copy link
Contributor Author

/pulsarbot run-failure-checks

@aloyszhang aloyszhang merged commit 71598c1 into apache:master Mar 21, 2024
100 checks passed
@chenhongSZ chenhongSZ deleted the make-maxMessageSize-non-static branch March 26, 2024 06:23
lhotari pushed a commit that referenced this pull request Mar 27, 2024
…ccessing multiple clusters in the same process (#22306)

Co-authored-by: atomchchen <atomchchen@tencent.com>
(cherry picked from commit 71598c1)
lhotari pushed a commit that referenced this pull request Mar 27, 2024
…ccessing multiple clusters in the same process (#22306)

Co-authored-by: atomchchen <atomchchen@tencent.com>
(cherry picked from commit 71598c1)
mukesh-ctds pushed a commit to datastax/pulsar that referenced this pull request Apr 15, 2024
…ccessing multiple clusters in the same process (apache#22306)

Co-authored-by: atomchchen <atomchchen@tencent.com>
(cherry picked from commit 71598c1)
(cherry picked from commit bff6ea2)
mukesh-ctds pushed a commit to datastax/pulsar that referenced this pull request Apr 17, 2024
…ccessing multiple clusters in the same process (apache#22306)

Co-authored-by: atomchchen <atomchchen@tencent.com>
(cherry picked from commit 71598c1)
(cherry picked from commit bff6ea2)
mukesh-ctds pushed a commit to datastax/pulsar that referenced this pull request Apr 17, 2024
…ccessing multiple clusters in the same process (apache#22306)

Co-authored-by: atomchchen <atomchchen@tencent.com>
(cherry picked from commit 71598c1)
(cherry picked from commit bff6ea2)
mukesh-ctds pushed a commit to datastax/pulsar that referenced this pull request Apr 19, 2024
…ccessing multiple clusters in the same process (apache#22306)

Co-authored-by: atomchchen <atomchchen@tencent.com>
(cherry picked from commit 71598c1)
(cherry picked from commit bff6ea2)
srinath-ctds pushed a commit to datastax/pulsar that referenced this pull request Apr 23, 2024
…ccessing multiple clusters in the same process (apache#22306)

Co-authored-by: atomchchen <atomchchen@tencent.com>
(cherry picked from commit 71598c1)
(cherry picked from commit bff6ea2)
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.

9 participants