G
YouTube TikTok Games
Skip to content

[fix][client]Fixed getting an incorrect maxMessageSize value when accessing multiple clusters in the same process - #22306

Merged
aloyszhang merged 5 commits into
apache:masterfrom
chenhongSZ:make-maxMessageSize-non-static
Mar 21, 2024
Merged

aloyszhang merged 5 commits into
apache:masterfrom
chenhongSZ:make-maxMessageSize-non-static

Conversation

@chenhongSZ

@chenhongSZ chenhongSZ commented Mar 19, 2024 •

Copy link
Copy Markdown
Contributor

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:

@github-actions

Copy link
Copy Markdown

@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

@lhotari lhotari left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM, good work!

@lhotari

lhotari commented Mar 19, 2024

Copy link
Copy Markdown
Member

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
Copy Markdown
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

lhotari commented Mar 20, 2024

Copy link
Copy Markdown
Member

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

@chenhongSZ

Copy link
Copy Markdown
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
Copy Markdown
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

codecov-commenter commented Mar 21, 2024 •

Copy link
Copy Markdown

Codecov Report

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

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

Files with missing lines Patch % Lines
...sar/client/impl/AbstractBatchMessageContainer.java 50.00% 0 Missing and 2 partials ⚠️
.../pulsar/client/impl/BatchMessageContainerImpl.java 60.00% 1 Missing and 1 partial ⚠️
...va/org/apache/pulsar/client/impl/ConsumerImpl.java 50.00% 0 Missing and 1 partial ⚠️
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 with missing lines 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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@chenhongSZ

Copy link
Copy Markdown
Contributor Author

/pulsarbot run-failure-checks

@aloyszhang
aloyszhang merged commit 71598c1 into apache:master Mar 21, 2024
@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)
hanmz pushed a commit to hanmz/pulsar that referenced this pull request Feb 12, 2025
…ccessing multiple clusters in the same process (apache#22306)

Co-authored-by: atomchchen <atomchchen@tencent.com>
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