Skip to content

[fix][broker] Closed topics won't be removed from the cache #23884

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
Merged
shibd merged 5 commits into from
Feb 6, 2025
Merged

[fix][broker] Closed topics won't be removed from the cache #23884

shibd merged 5 commits into from
Feb 6, 2025

Conversation

@shibd
Copy link
Member

@shibd shibd commented Jan 23, 2025 •
edited
Loading

Motivation

When transaction component init fails, it will try to close the topic, but maybe not remove topic from cache.

time thread createTransactionBufferSystemTopicClient thread create topic
1 topic ledger open complete and new PersistentTopic object
2 new TransactionBuffer
3 Try get AbortedTxnProcessor
4 Failed to create snapshot writer and close topic
5 set topic fenced and closing
6 try remove topic from broker cache, since the topic feature is not done, will ignore it
7 Topic future success complete

Modifications

  • Add topic create future to topic.
  • When remove a topic, call removeTopicFutureFromCache and passed topic future.
  • Remove method removeTopicFromCache(Topic) that intrudce from [fix][broker]Consumer can't consume messages because there has two sames topics in one broker #17526
    • At that time, there could have been two topicFutures for the same topic simultaneously (one creating, one closing). However, this case no longer occurs because we fixed it in #22860, ensuring that a topic cannot be removed until it is fully created or closed.

Verifying this change

  • You can run testNoOrphanClosedTopicIfTxnInternalFailed to reproduce this.

Documentation

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

Test PR

shibd#53

@shibd shibd self-assigned this Jan 23, 2025
@github-actions github-actions bot added the doc-not-needed Your PR changes do not impact docs label Jan 23, 2025
@codecov-commenter
Copy link

codecov-commenter commented Jan 23, 2025 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 74.22%. Comparing base (bbc6224) to head (c61c192).
Report is 878 commits behind head on master.

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff              @@
##             master   #23884      +/-   ##
============================================
+ Coverage     73.57%   74.22%   +0.65%     
+ Complexity    32624    32254     -370     
============================================
  Files          1877     1853      -24     
  Lines        139502   143726    +4224     
  Branches      15299    16333    +1034     
============================================
+ Hits         102638   106685    +4047     
+ Misses        28908    28640     -268     
- Partials       7956     8401     +445
Flag Coverage Δ
inttests 26.72% <71.42%> (+2.14%) ⬆️
systests 23.25% <21.42%> (-1.08%) ⬇️
unittests 73.74% <100.00%> (+0.90%) ⬆️

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

Files with missing lines Coverage Δ
...rg/apache/pulsar/broker/service/AbstractTopic.java 87.77% <ø> (-0.22%) ⬇️
...rg/apache/pulsar/broker/service/BrokerService.java 84.97% <100.00%> (+4.19%) ⬆️
...ransaction/buffer/impl/TopicTransactionBuffer.java 85.75% <100.00%> (-2.00%) ⬇️

... and 1029 files with indirect coverage changes

lhotari
lhotari requested changes Jan 23, 2025
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.

Please reduce code duplication

@shibd
Copy link
Member Author

shibd commented Jan 24, 2025 •
edited
Loading

hi, @lhotari @BewareMyPower @dao-jun Thanks for details review,

After discussing with @poorbarcode , we've decided to pass createFuture into the topic and call topic.remove(topic, topicFuture) to direct remove it, I'll push a new version later.

@shibd shibd marked this pull request as draft January 24, 2025 10:08
@shibd shibd force-pushed the fix_txn_topic_stuck branch from 85302cb to 6cdf378 Compare January 26, 2025 02:10
@shibd shibd marked this pull request as ready for review January 26, 2025 02:17
@shibd
Copy link
Member Author

shibd commented Jan 26, 2025

/pulsarbot rerun-failure-checks

RobertIndie
RobertIndie reviewed Jan 26, 2025
View reviewed changes
Copy link
Member

@RobertIndie RobertIndie left a comment

Choose a reason for hiding this comment

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

Overall looks good to me. Thanks.

@shibd shibd requested a review from RobertIndie January 26, 2025 11:18
lhotari
lhotari requested changes Jan 27, 2025
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.

Please check the review comments.

@shibd shibd requested a review from lhotari February 4, 2025 09:36
lhotari
lhotari requested changes Feb 5, 2025
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.

Please check comment about the comment.

@shibd shibd requested a review from lhotari February 5, 2025 14:15
lhotari
lhotari approved these changes Feb 5, 2025
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

@shibd shibd merged commit 8a40b30 into apache:master Feb 6, 2025
51 of 52 checks passed
shibd added a commit that referenced this pull request Feb 6, 2025
@shibd
[fix][broker] Closed topics won't be removed from the cache (#23884)
5d35d41 
(cherry picked from commit 8a40b30)
shibd added a commit that referenced this pull request Feb 6, 2025
@shibd
[fix][broker] Closed topics won't be removed from the cache (#23884)
e1d8c7d 
(cherry picked from commit 8a40b30)
shibd added a commit that referenced this pull request Feb 6, 2025
@shibd
[fix][broker] Closed topics won't be removed from the cache (#23884)
ccf7284 
(cherry picked from commit 8a40b30)
nodece pushed a commit to ascentstream/pulsar that referenced this pull request Feb 10, 2025
@shibd @nodece
[fix][broker] Closed topics won't be removed from the cache (apache#2…
8ec413a 
…3884)

(cherry picked from commit 8a40b30)
hanmz pushed a commit to hanmz/pulsar that referenced this pull request Feb 12, 2025
@shibd @hanmz
[fix][broker] Closed topics won't be removed from the cache (apache#2…
564601a 
…3884)
nikhil-ctds pushed a commit to datastax/pulsar that referenced this pull request Feb 19, 2025
@shibd @nikhil-ctds
[fix][broker] Closed topics won't be removed from the cache (apache#2…
78b843d 
…3884)

(cherry picked from commit 8a40b30)
(cherry picked from commit 5d35d41)
srinath-ctds pushed a commit to datastax/pulsar that referenced this pull request Feb 24, 2025
@shibd @srinath-ctds
[fix][broker] Closed topics won't be removed from the cache (apache#2…
86f1982 
…3884)

(cherry picked from commit 8a40b30)
(cherry picked from commit 5d35d41)
@BewareMyPower BewareMyPower mentioned this pull request Jul 10, 2025
Closed
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@lhotari lhotari lhotari approved these changes

@BewareMyPower BewareMyPower BewareMyPower approved these changes

@poorbarcode poorbarcode poorbarcode approved these changes

@Technoboy- Technoboy- Awaiting requested review from Technoboy-

@gaoran10 gaoran10 Awaiting requested review from gaoran10

@congbobo184 congbobo184 Awaiting requested review from congbobo184

@liangyepianzhou liangyepianzhou Awaiting requested review from liangyepianzhou

@dao-jun dao-jun Awaiting requested review from dao-jun

@RobertIndie RobertIndie Awaiting requested review from RobertIndie

Assignees

@shibd shibd

Labels

cherry-picked/branch-3.0 cherry-picked/branch-3.3 cherry-picked/branch-4.0 doc-not-needed Your PR changes do not impact docs ready-to-test release/3.0.10 release/3.3.5 release/4.0.3 type/bug The PR fixed a bug or issue reported a bug

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@shibd @codecov-commenter @lhotari @RobertIndie @BewareMyPower @dao-jun @poorbarcode