Skip to content

Conversation

@BewareMyPower
Copy link
Contributor

@BewareMyPower BewareMyPower commented Mar 18, 2025

Fixes #23825

Motivation

It seems that the PIP-379 breaks #7266. For example, given the following configs:

  • The consumer whose available permits is 10
  • There are 10 entries available, each entry's batch size is 10.

When subscriptionSharedUseClassicPersistentImplementation is false (by default), 10 entries will be dispatched to the consumer. When it's true (the behavior before 4.0), only 1 entry will be dispatched to the consumer.

Modifications

Fix the maxEntriesInThisBatch calculation in PersistentDispatcherMultipleConsumers. In testBatchMessageDispatchingAccordingToPermits, wait until the entries are dispatched to consumers. Then this test would fail without this fix.

Documentation

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

Matching PR in forked repository

PR in forked repository:

@BewareMyPower BewareMyPower added type/bug The PR fixed a bug or issue reported a bug release/4.0.4 labels Mar 18, 2025
@BewareMyPower BewareMyPower added this to the 4.1.0 milestone Mar 18, 2025
@BewareMyPower BewareMyPower self-assigned this Mar 18, 2025
@github-actions github-actions bot added the doc-not-needed Your PR changes do not impact docs label Mar 18, 2025
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.

Thanks for spotting this regression @BewareMyPower. I added one suggestion.

@BewareMyPower BewareMyPower marked this pull request as draft March 19, 2025 01:53
@BewareMyPower BewareMyPower force-pushed the bewaremypower/flaky-testBatchMessage branch from 32019f4 to 3ca7df6 Compare March 19, 2025 02:12
@BewareMyPower BewareMyPower marked this pull request as ready for review March 19, 2025 02:24
@codecov-commenter
Copy link

codecov-commenter commented Mar 19, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 74.18%. Comparing base (bbc6224) to head (0a960a2).
Report is 977 commits behind head on master.

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff              @@
##             master   #24092      +/-   ##
============================================
+ Coverage     73.57%   74.18%   +0.61%     
+ Complexity    32624    32427     -197     
============================================
  Files          1877     1863      -14     
  Lines        139502   144312    +4810     
  Branches      15299    16457    +1158     
============================================
+ Hits         102638   107063    +4425     
+ Misses        28908    28773     -135     
- Partials       7956     8476     +520     
Flag Coverage Δ
inttests 26.69% <100.00%> (+2.10%) ⬆️
systests 23.20% <100.00%> (-1.13%) ⬇️
unittests 73.69% <100.00%> (+0.84%) ⬆️

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

Files with missing lines Coverage Δ
...sistent/PersistentDispatcherMultipleConsumers.java 74.92% <100.00%> (+0.60%) ⬆️

... and 1062 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.

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

@lhotari lhotari merged commit 2f6345d into apache:master Mar 19, 2025
52 checks passed
lhotari pushed a commit that referenced this pull request Mar 20, 2025
…ng to consumer permits (#24092)

(cherry picked from commit 2f6345d)
nikhil-ctds pushed a commit to datastax/pulsar that referenced this pull request Mar 28, 2025
…ng to consumer permits (apache#24092)

(cherry picked from commit 2f6345d)
(cherry picked from commit 2fb9cb5)
nikhil-ctds pushed a commit to datastax/pulsar that referenced this pull request Mar 29, 2025
…ng to consumer permits (apache#24092)

(cherry picked from commit 2f6345d)
(cherry picked from commit 2fb9cb5)
srinath-ctds pushed a commit to datastax/pulsar that referenced this pull request Mar 31, 2025
…ng to consumer permits (apache#24092)

(cherry picked from commit 2f6345d)
(cherry picked from commit 2fb9cb5)
walkinggo pushed a commit to walkinggo/pulsar that referenced this pull request Oct 8, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cherry-picked/branch-4.0 doc-not-needed Your PR changes do not impact docs release/4.0.4 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.

Flaky-test: BatchMessageTest.testBatchMessageDispatchingAccordingToPermits

6 participants