[Broker] Fix LeaderElectionService.getCurrentLeader and add support for empheralOwner in MockZooKeeper by lhotari · Pull Request #13066 · apache/pulsar

lhotari · 2021-12-01T13:36:24Z

Motivation

LeaderElectionService.getCurrentLeader() always returned Optional.empty() on all other than the leader broker.
This has several severe consequences in the Pulsar Broker code base.

When a lookup is made to a topic that isn't currently loaded, the decision will be made in a distributed fashion on the follower brokers since the information about the leader broker is missing (because LeaderElectionService.getCurrentLeader() always returned Optional.empty()). This leads to races when assigning the topic ownership to a broker since the decision isn't made centrally on the leader broker.
I'll provide a separate PR for improving and fixing the topic ownership assignment. That PR is #13069 and it contains a failing test case for the topic ownership assignment / lookup problem.

Modifications

Add support for empheralOwner in MockZooKeeper
Add MultiBrokerBaseTest base test class for writing tests that involve multiple brokers
Add test case that fails without the fix
Make the fix to LeaderElectionImpl

eolivelli

LGTM

@merlimat can you please take a look ?

@lhotari my understanding is that this patch should go in 2.8 as well

lhotari · 2021-12-01T15:24:42Z

my understanding is that this patch should go in 2.8 as well

@eolivelli yes, 2.8.x and 2.9.x are also impacted.

merlimat · 2021-12-01T17:41:41Z

BTW: I also plan to get rid of MockZookeeper in the short term :)

lhotari · 2021-12-01T17:46:20Z

BTW: I also plan to get rid of MockZookeeper in the short term :)

@merlimat +1, that would reduce the risk that it's the MockZookeeper that causes certain behavior in tests.

merlimat · 2021-12-01T19:13:07Z

BTW: I also plan to get rid of MockZookeeper in the short term :)

@merlimat +1, that would reduce the risk that it's the MockZookeeper that causes certain behavior in tests.

Yes, the way it is, it doesn't help much. We've been chasing down many times issues that stemmed from it semantic being different from the real ZK in subtle ways.

For unit tests, I believe the LocalMemory is a better alternative, although we need to add the ephemeral owner there too.

lhotari · 2021-12-01T21:00:13Z

Closing and reopening to get fix to flaky test for builds.

…other brokers

eolivelli

LGTM

eolivelli · 2021-12-02T17:22:03Z

+import org.testng.annotations.AfterClass;
+import org.testng.annotations.BeforeClass;
+
+public abstract class MultiBrokerBaseTest extends MockedPulsarServiceBaseTest {


nit:
this is actually not a "Test", we should not name it "**Test"

btw this is the current code style in Pulsar, so no big deal, just sharing this thought

eolivelli · 2021-12-02T17:28:21Z

@linlinnn we must pick this patch to branch-2.8 before cutting a release.

2.8 is too unstable due to this problem

…or empheralOwner in MockZooKeeper (#13066) * Add leader election unit test that uses multiple brokers * Support empheralOwner in MockZooKeeper * Use unique sessions for all brokers * Add failing test case for leader broker information not available on other brokers * Add test for reading the current leader * Fix issue in leader election * Address review feedback: make methods static * Use unique-session only in MultiBrokerBaseTests * Move tenant and namespace creation to it's own method * Improve cleanup * Add alwaysRun to BeforeClass * Fix leaks in locking in MockZooKeeper * Reduce code duplication * Fix NPE when CreateMode is null (cherry picked from commit 36a45ee)

lhotari · 2021-12-02T18:57:53Z

@eolivelli @linlinnn I cherry picked to branch-2.8 and branch-2.9 .

lhotari · 2021-12-02T19:03:08Z

@linlinnn @eolivelli PR #13069 would also be necessary for 2.8 to fix the problems with topic ownership assignments.

…or empheralOwner in MockZooKeeper (apache#13066) * Add leader election unit test that uses multiple brokers * Support empheralOwner in MockZooKeeper * Use unique sessions for all brokers * Add failing test case for leader broker information not available on other brokers * Add test for reading the current leader * Fix issue in leader election * Address review feedback: make methods static * Use unique-session only in MultiBrokerBaseTests * Move tenant and namespace creation to it's own method * Improve cleanup * Add alwaysRun to BeforeClass * Fix leaks in locking in MockZooKeeper * Reduce code duplication * Fix NPE when CreateMode is null (cherry picked from commit 36a45ee) (cherry picked from commit 72690be)

- fixes cherry picking of #13066 in 72690be

- fixes cherry picking of apache#13066 in 72690be (cherry picked from commit 59d8ad9)

…951) Fix https://github.com/streamnative/kop/runs/4425667410?check_suite_focus=true#step:5:283 #950 ### Motivation MockZookeeper API was changed in apache/pulsar#13066, the `CreateMode` shouldn't be null. We should use MetadataStore instead of mockZooKeeper since KoP already remove all zkClient usage. ### Modifications Use MetadataStore instead of mockZooKeeper in ProducerIdManagerTest

Fix #950 ### Motivation apache/pulsar#13066 introduced a breaking change to the `MockZooKeeper` API and causes test failure. ### Modifications * Fix failure test * Upgrade pulsar version to 2.8.1.0

…or empheralOwner in MockZooKeeper (apache#13066) * Add leader election unit test that uses multiple brokers * Support empheralOwner in MockZooKeeper * Use unique sessions for all brokers * Add failing test case for leader broker information not available on other brokers * Add test for reading the current leader * Fix issue in leader election * Address review feedback: make methods static * Use unique-session only in MultiBrokerBaseTests * Move tenant and namespace creation to it's own method * Improve cleanup * Add alwaysRun to BeforeClass * Fix leaks in locking in MockZooKeeper * Reduce code duplication * Fix NPE when CreateMode is null

lhotari · 2022-09-01T15:16:53Z

Further improvement in #17401

lhotari added type/bug The PR fixed a bug or issue reported a bug area/broker doc-not-needed Your PR changes do not impact docs labels Dec 1, 2021

lhotari added this to the 2.10.0 milestone Dec 1, 2021

lhotari requested review from codelipenghui, eolivelli, merlimat and rdhabalia December 1, 2021 13:36

lhotari self-assigned this Dec 1, 2021

lhotari changed the title ~~[Broker] Fix LeaderElectionService.getCurrentLeader and support for empheralOwner in MockZooKeeper~~ [Broker] Fix LeaderElectionService.getCurrentLeader and add support for empheralOwner in MockZooKeeper Dec 1, 2021

lhotari mentioned this pull request Dec 1, 2021

[Broker] Fix and improve topic ownership assignment #13069

Merged

eolivelli approved these changes Dec 1, 2021

View reviewed changes

Comment thread testmocks/src/main/java/org/apache/zookeeper/MockZooKeeper.java Outdated

Comment thread testmocks/src/main/java/org/apache/zookeeper/MockZooKeeper.java Outdated

lhotari added release/2.8.3 release/2.9.1 labels Dec 1, 2021

merlimat approved these changes Dec 1, 2021

View reviewed changes

lhotari closed this Dec 1, 2021

lhotari reopened this Dec 1, 2021

lhotari force-pushed the lh-fix-LeaderElectionService.getCurrentLeader branch from dad26de to e266578 Compare December 2, 2021 12:19

lhotari added 7 commits December 2, 2021 17:28

Add leader election unit test that uses multiple brokers

9a838b7

Support empheralOwner in MockZooKeeper

f5a081c

Use unique sessions for all brokers

c1478c8

Add failing test case for leader broker information not available on …

0cb7148

…other brokers

Add test for reading the current leader

ed055f6

Fix issue in leader election

ebeb584

Address review feedback: make methods static

0d8dd59

lhotari force-pushed the lh-fix-LeaderElectionService.getCurrentLeader branch from e266578 to f68da4c Compare December 2, 2021 15:29

Fix NPE when CreateMode is null

331329e

lhotari mentioned this pull request Dec 2, 2021

[Tests] LockManagerImpl.close hangs and causes the test run to timeout eventually #13092

Closed

lhotari requested a review from eolivelli December 2, 2021 17:09

eolivelli approved these changes Dec 2, 2021

View reviewed changes

lhotari merged commit 36a45ee into apache:master Dec 2, 2021

lhotari added release/2.8.2 and removed release/2.8.3 labels Dec 2, 2021

lhotari added the cherry-picked/branch-2.8 Archived: 2.8 is end of life label Dec 2, 2021

lhotari added the cherry-picked/branch-2.9 Archived: 2.9 is end of life label Dec 2, 2021

lhotari mentioned this pull request Dec 2, 2021

MaxRedirectException occurs when attempting to delete a subscription that was recently created and then had its topic destroyed #12552

Closed

lhotari added a commit that referenced this pull request Dec 3, 2021

Fix invalid MockZooKeeper tests which had null createMode

59d8ad9

- fixes cherry picking of #13066 in 72690be

lhotari added a commit to lhotari/pulsar that referenced this pull request Dec 3, 2021

Fix invalid MockZooKeeper tests which had null createMode

d8948bc

- fixes cherry picking of apache#13066 in 72690be (cherry picked from commit 59d8ad9)

BewareMyPower mentioned this pull request Dec 6, 2021

Fix NPE problems caused by mockZooKeeper API changes when dump version to 2.8.1.26 streamnative/kop#951

Merged

BewareMyPower mentioned this pull request Dec 6, 2021

Bump version to 2.8.1.26 streamnative/kop#952

Closed

Demogorgon314 mentioned this pull request Dec 6, 2021

Bump pulsar version to 2.8.1.0 streamnative/kop#953

Merged

Demogorgon314 mentioned this pull request Jan 18, 2022

[Issue #11348][pulsar-broker] Fix can not get leader broker in follower brokers #11353

Merged

lhotari mentioned this pull request Jan 29, 2025

[fix][meta] Fix ephemeral Zookeeper put which creates a persistent znode #23902

Closed

15 tasks

Uh oh!

Conversation

lhotari commented Dec 1, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Motivation

Modifications

Uh oh!

eolivelli left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

lhotari commented Dec 1, 2021

Uh oh!

merlimat commented Dec 1, 2021

Uh oh!

lhotari commented Dec 1, 2021

Uh oh!

merlimat commented Dec 1, 2021

Uh oh!

lhotari commented Dec 1, 2021

Uh oh!

eolivelli left a comment

Choose a reason for hiding this comment

Uh oh!

eolivelli Dec 2, 2021

Choose a reason for hiding this comment

Uh oh!

eolivelli commented Dec 2, 2021

Uh oh!

lhotari commented Dec 2, 2021

Uh oh!

lhotari commented Dec 2, 2021

Uh oh!

lhotari commented Sep 1, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

lhotari commented Dec 1, 2021 •

edited

Loading