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][test] Fix testLargeMessage doesn't respect the parameter clientSizeMaxMessageSize #18452

Merged
merged 1 commit into from
Nov 15, 2022

Conversation

RobertIndie
Copy link
Member

@RobertIndie RobertIndie commented Nov 14, 2022

Motivation

#14382 introduced the client-side maxMessageSize for the message chunking. But the parameter clientSizeMaxMessageSize in testLargeMessage is not respected in that PR. This PR fixes this issue.

Modifications

Verifying this change

This change is already covered by existing tests, such as testLargeMessage.

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: RobertIndie#8

@RobertIndie RobertIndie added type/bug The PR fixed a bug or issue reported a bug area/test labels Nov 14, 2022
@RobertIndie RobertIndie added this to the 2.12.0 milestone Nov 14, 2022
@RobertIndie RobertIndie self-assigned this Nov 14, 2022
@github-actions github-actions bot added the doc-not-needed Your PR changes do not impact docs label Nov 14, 2022
@codecov-commenter
Copy link

codecov-commenter commented Nov 14, 2022

Codecov Report

Merging #18452 (96153ed) into master (fdf86d3) will increase coverage by 1.50%.
The diff coverage is 0.00%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master   #18452      +/-   ##
============================================
+ Coverage     45.67%   47.17%   +1.50%     
- Complexity    10075    10418     +343     
============================================
  Files           693      693              
  Lines         67940    67942       +2     
  Branches       7273     7275       +2     
============================================
+ Hits          31030    32053    +1023     
+ Misses        33333    32313    -1020     
+ Partials       3577     3576       -1     
Flag Coverage Δ
unittests 47.17% <0.00%> (+1.50%) ⬆️

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

Impacted Files Coverage Δ
...va/org/apache/pulsar/client/impl/ConsumerImpl.java 15.07% <0.00%> (-0.03%) ⬇️
...apache/pulsar/client/impl/AutoClusterFailover.java 73.33% <0.00%> (-2.23%) ⬇️
...oker/service/schema/SchemaRegistryServiceImpl.java 61.26% <0.00%> (-1.81%) ⬇️
...broker/service/schema/BookkeeperSchemaStorage.java 73.92% <0.00%> (-0.31%) ⬇️
...che/bookkeeper/mledger/impl/ManagedCursorImpl.java 37.11% <0.00%> (-0.06%) ⬇️
...pache/pulsar/broker/admin/impl/NamespacesBase.java 63.89% <0.00%> (+0.06%) ⬆️
...he/pulsar/client/impl/MultiTopicsConsumerImpl.java 23.07% <0.00%> (+0.12%) ⬆️
...che/bookkeeper/mledger/impl/ManagedLedgerImpl.java 54.11% <0.00%> (+0.13%) ⬆️
...oker/service/nonpersistent/NonPersistentTopic.java 57.63% <0.00%> (+0.17%) ⬆️
.../org/apache/pulsar/broker/admin/v2/Namespaces.java 59.49% <0.00%> (+0.20%) ⬆️
... and 60 more

@tisonkun
Copy link
Member

Thanks for taking care of this! Merging...

@tisonkun tisonkun merged commit 034e6a4 into apache:master Nov 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/test doc-not-needed Your PR changes do not impact docs ready-to-test type/bug The PR fixed a bug or issue reported a bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants