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.

Sign up for GitHub

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

Jump to bottom

[fix][broker] Check cursor state before adding it to the waitingCursors #22191

Merged
merged 4 commits into from
Mar 27, 2024
Merged

[fix][broker] Check cursor state before adding it to the waitingCursors #22191

merged 4 commits into from
Mar 27, 2024

Conversation

Technoboy-
Copy link
Contributor

Fixes #22157

Motivation

The root cause :
There is a race condition between the consumer.close and checkForNewEntries.
When the consumer is closed, the waitingCursor may be empty.

pulsar/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentSubscription.java

Lines 298 to 313 in ccc2ea6

public synchronized void removeConsumer(Consumer consumer, boolean isResetCursor) throws BrokerServiceException {
cursor.updateLastActive();
if (dispatcher != null) {
dispatcher.removeConsumer(consumer);
}
// preserve accumulative stats form removed consumer
ConsumerStatsImpl stats = consumer.getStats();
bytesOutFromRemovedConsumers.add(stats.bytesOutCounter);
msgOutFromRemovedConsumer.add(stats.msgOutCounter);
if (dispatcher != null && dispatcher.getConsumers().isEmpty()) {
deactivateCursor();
topic.getManagedLedger().removeWaitingCursor(cursor);
if (!cursor.isDurable()) {
.
Then after executing line-311, the cursor was added to waitingCursor.

Documentation

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

@Technoboy- Technoboy- self-assigned this Mar 4, 2024
@Technoboy- Technoboy- added this to the 3.3.0 milestone Mar 4, 2024
@github-actions github-actions bot added the doc-not-needed Your PR changes do not impact docs label Mar 4, 2024
lhotari
lhotari approved these changes Mar 4, 2024
View reviewed changes
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 @Technoboy-

@lhotari lhotari merged commit b702d44 into apache:master Mar 27, 2024
49 of 50 checks passed
lhotari pushed a commit that referenced this pull request Mar 27, 2024
@Technoboy- @lhotari
[fix][broker] Check cursor state before adding it to the `waitingCurs…
ba8ff27 
…ors` (#22191)

(cherry picked from commit b702d44)
lhotari pushed a commit that referenced this pull request Mar 27, 2024
@Technoboy- @lhotari
[fix][broker] Check cursor state before adding it to the `waitingCurs…
622c87d 
…ors` (#22191)

(cherry picked from commit b702d44)
Technoboy- added a commit to Technoboy-/pulsar that referenced this pull request Apr 1, 2024
@Technoboy-
[fix][broker] Check cursor state before adding it to the `waitingCurs…
3dea6a2 
…ors` (apache#22191)
@KannarFr KannarFr mentioned this pull request Apr 4, 2024
Closed
3 tasks
@lhotari lhotari mentioned this pull request Apr 8, 2024
Merged
4 tasks
@lhotari
Copy link
Member

lhotari commented Apr 9, 2024 •
edited
Loading

#22454 should be cherry-picked too if this is cherry-picked since this PR introduced a regression #22435 which is fixed by #22454.

lhotari pushed a commit that referenced this pull request Apr 9, 2024
@Technoboy- @lhotari
[fix][broker] Check cursor state before adding it to the `waitingCurs…
afe7bd5 
…ors` (#22191)

(cherry picked from commit b702d44)
mukesh-ctds pushed a commit to datastax/pulsar that referenced this pull request Apr 15, 2024
@Technoboy- @mukesh-ctds
[fix][broker] Check cursor state before adding it to the `waitingCurs…
8682575 
…ors` (apache#22191)

(cherry picked from commit b702d44)
(cherry picked from commit ba8ff27)
mukesh-ctds pushed a commit to datastax/pulsar that referenced this pull request Apr 17, 2024
@Technoboy- @mukesh-ctds
[fix][broker] Check cursor state before adding it to the `waitingCurs…
0d6f128 
…ors` (apache#22191)

(cherry picked from commit b702d44)
(cherry picked from commit ba8ff27)
mukesh-ctds pushed a commit to datastax/pulsar that referenced this pull request Apr 17, 2024
@Technoboy- @mukesh-ctds
[fix][broker] Check cursor state before adding it to the `waitingCurs…
7d3675b 
…ors` (apache#22191)

(cherry picked from commit b702d44)
(cherry picked from commit ba8ff27)
mukesh-ctds pushed a commit to datastax/pulsar that referenced this pull request Apr 19, 2024
@Technoboy- @mukesh-ctds
[fix][broker] Check cursor state before adding it to the `waitingCurs…
51fab32 
…ors` (apache#22191)

(cherry picked from commit b702d44)
(cherry picked from commit ba8ff27)
srinath-ctds pushed a commit to datastax/pulsar that referenced this pull request Apr 23, 2024
@Technoboy- @srinath-ctds
[fix][broker] Check cursor state before adding it to the `waitingCurs…
95c0675 
…ors` (apache#22191)

(cherry picked from commit b702d44)
(cherry picked from commit ba8ff27)
@lhotari lhotari mentioned this pull request May 3, 2024
Closed
3 tasks
pgier pushed a commit to pgier/pulsar that referenced this pull request Aug 23, 2024
@Technoboy- @pgier
[fix][broker] Check cursor state before adding it to the `waitingCurs…
8fefb62 
…ors` (apache#22191)

(cherry picked from commit b702d44)
(cherry picked from commit ba8ff27)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dao-jun dao-jun dao-jun approved these changes

@lhotari lhotari lhotari approved these changes

Assignees

@Technoboy- Technoboy-

Labels
cherry-picked/branch-3.0 cherry-picked/branch-3.1 cherry-picked/branch-3.2 doc-not-needed Your PR changes do not impact docs ready-to-test release/2.10.7 release/2.11.5 release/3.0.4 release/3.1.4 release/3.2.2
Projects
None yet
Milestone
3.3.0
Development

Successfully merging this pull request may close these issues.

[Bug] Broker memory leak
3 participants
@Technoboy- @lhotari @dao-jun