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] [broker] Internal reader of __change_events can not started after metadata store session rebuilt #23018

Merged
merged 9 commits into from
Jul 29, 2024

Conversation

poorbarcode
Copy link
Contributor

@poorbarcode poorbarcode commented Jul 9, 2024

Motivation & Modifications

1. Fix the issue below

  • settings: enable systemTopic & topicLevelPolicies.
  • Own topics
    • broker-2 loads a topic-a up.
    • broker-2 loads system topic({ns}/__change_events) up.
  • The ZK session expires.
  • Bundle shedding
    • topic-a was transferred to broker-0
    • the system topic({ns}/__change_events) was transferred to broker-0
  • (Highlight) Issue occurs on broker-2
    • The internal reader for {ns}/__change_events lookup the topic's owner
    • broker-2 responds broker-2 due to its OwnershipCache contains the old value.
    • The internal reader for {ns}/__change_events can not be created successfully, all topics under this namespace can not work.
    • The consumers/producers who call lookup to broker-2 will get a wrong value, and keep getting a "Please redo the lookup" error.

2. Add a test to confirm the topic will be unloaded after the metadata store session is rebuilt.

  • Broker load a topic up.
  • The ZK session expires.
  • The ZK session rebuild.
  • Verify: The topics that the broker owned will be unloaded

Documentation

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

Matching PR in forked repository

PR in forked repository: x

@codecov-commenter
Copy link

codecov-commenter commented Jul 9, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 73.45%. Comparing base (bbc6224) to head (e439b97).
Report is 475 commits behind head on master.

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff              @@
##             master   #23018      +/-   ##
============================================
- Coverage     73.57%   73.45%   -0.13%     
- Complexity    32624    33216     +592     
============================================
  Files          1877     1917      +40     
  Lines        139502   144072    +4570     
  Branches      15299    15743     +444     
============================================
+ Hits         102638   105824    +3186     
- Misses        28908    30131    +1223     
- Partials       7956     8117     +161     
Flag Coverage Δ
inttests 27.73% <80.00%> (+3.14%) ⬆️
systests 24.72% <0.00%> (+0.40%) ⬆️
unittests 72.52% <100.00%> (-0.33%) ⬇️

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

Files Coverage Δ
...dbalance/extensions/ExtensibleLoadManagerImpl.java 79.45% <ø> (-0.64%) ⬇️
...roker/loadbalance/impl/ModularLoadManagerImpl.java 84.98% <ø> (+<0.01%) ⬆️
...he/pulsar/metadata/impl/AbstractMetadataStore.java 84.94% <100.00%> (+0.38%) ⬆️

... and 509 files with indirect coverage changes

@heesung-sn
Copy link
Contributor

Can we cover the new load balancer behavior too?

@shibd shibd self-requested a review July 10, 2024 02:54
@poorbarcode
Copy link
Contributor Author

poorbarcode commented Jul 10, 2024

@heesung-sn

Can we cover the new load balancer behavior too?

Sure, I will push two following PRs to do this:

  • Enable system topic and topic-level policies. So far, this test can not pass when enabled system topic, I am checking why it does not work, and I will push a fix if there is an issue. Added into the current PR.
  • Enable ExtensibleLoadManager, since it depends on the system topic, this test will be added after the above one.

@poorbarcode poorbarcode requested a review from heesung-sn July 10, 2024 03:06
@poorbarcode poorbarcode changed the title [improve] [test] Add a test: topic will be unloaded after metadata store session rebuilt [fix] [broker] Topic should be unloaded after metadata store session expired Jul 10, 2024
@poorbarcode poorbarcode changed the title [fix] [broker] Topic should be unloaded after metadata store session expired [fix] [broker] Internal reader of __change_events can not started after metadata store session rebuilt Jul 10, 2024
@poorbarcode poorbarcode requested review from merlimat and removed request for merlimat July 10, 2024 13:06
@poorbarcode poorbarcode force-pushed the test/zk_session_expire branch from 68143c4 to bf8b6a9 Compare July 26, 2024 00:33
@poorbarcode poorbarcode requested a review from lhotari July 29, 2024 10:11
@Technoboy- Technoboy- merged commit b955c65 into apache:master Jul 29, 2024
55 checks passed
poorbarcode added a commit that referenced this pull request Jul 30, 2024
…er metadata store session rebuilt (#23018)

(cherry picked from commit b955c65)
poorbarcode added a commit that referenced this pull request Jul 30, 2024
…er metadata store session rebuilt (#23018)

(cherry picked from commit b955c65)
poorbarcode added a commit that referenced this pull request Jul 30, 2024
…er metadata store session rebuilt (#23018)

(cherry picked from commit b955c65)
nikhil-ctds pushed a commit to datastax/pulsar that referenced this pull request Jul 30, 2024
…er metadata store session rebuilt (apache#23018)

(cherry picked from commit b955c65)
(cherry picked from commit 0e59678)
srinath-ctds pushed a commit to datastax/pulsar that referenced this pull request Jul 30, 2024
…er metadata store session rebuilt (apache#23018)

(cherry picked from commit b955c65)
(cherry picked from commit 0e59678)
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.

7 participants