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][meta] Fix ephemeral handling of ZK nodes and fix MockZooKeeper ephemeral and ZK stat handling #23988

Merged
merged 56 commits into from
Feb 17, 2025
Merged

[fix][meta] Fix ephemeral handling of ZK nodes and fix MockZooKeeper ephemeral and ZK stat handling #23988

merged 56 commits into from
Feb 17, 2025

Conversation

lhotari
Copy link
Member

@lhotari lhotari commented Feb 14, 2025 •
edited
Loading

Motivation

This PR is a follow-up to #23984 to fix ephemeral status handling in ZKMetadataStore and to improve the ZooKeeper testing infrastructure.

A key addition is enabling pulsar-metadata tests to run against MockZooKeeper through a proper MetadataStoreProvider implementation, ensuring MockZooKeeper correctly implements all features that Pulsar depends on.

Modifications

  1. Fixed Production Code Issues:
  • Fixed ephemeral status handling in ZKMetadataStore to properly detect ephemeral nodes by using the correct comparison value (0L) for non-ephemeral (== persistent) nodes
  1. MockZooKeeper improvements and fixes:
  • Fixed ephemeral node handling and cleanup on session termination
  • Improved ZK stat handling with proper timestamps and version numbers
  • Added proper session ID management and consistent thread safety
  • Fixed persistent watcher cleanup on session termination
  • Fixed getChildren implementation and removed code duplication
  • Fixed ordering issues by migrating to use a single threaded executor model
    • This allowed to remove locks and synchronization in most cases
  1. Test Framework Improvements::
  • Implemented MockZooKeeperMetadataStoreProvider to allow pulsar-metadata tests to use MockZooKeeper
    • This ensures that MockZooKeeper correctly implements all MetadataStore features that Pulsar depends on
  • Enhanced PulsarTestContext to support toggling between MockZooKeeper and real ZK (TestZKServer) implementations
  • Added ability to run MockedPulsarServiceBaseTest tests with both MockZooKeeper and real ZK (TestZKServer)

There are some unrelated NPE fixes and improvements for better exception messages. I faced those issues while debugging BrokerServiceTest and noticed those exceptions in the execution logs. Some of the exceptions were due to invalid tests, however the improvements will be useful also later.

Documentation

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

@lhotari lhotari requested a review from heesung-sn February 14, 2025 15:40
@github-actions github-actions bot added the doc-not-needed Your PR changes do not impact docs label Feb 14, 2025
@lhotari lhotari mentioned this pull request Feb 14, 2025
Closed
15 tasks
@lhotari lhotari added this to the 4.1.0 milestone Feb 14, 2025
@lhotari lhotari added release/3.0.10 release/3.3.5 release/4.0.3 release/blocker Indicate the PR or issue that should block the release until it gets resolved labels Feb 14, 2025
lhotari added 15 commits February 17, 2025 14:11
@lhotari
[fix][metadata] Fix ephemeral status of ZK nodes
ee22969
@lhotari
Run pulsar-metadata tests with MockZooKeeper
c45c6d6
@lhotari
Improve test names
cd5610a
@lhotari
Fix Stat handling in MockZooKeeper
244a65d
@lhotari
Cleanup watchers tied to a particular ZK session in MockZooKeeper
d9ec2d1
@lhotari
Improve MockZNode handling
153289e
@lhotari
Pass the overridden session id to the executor thread
69ca2df
@lhotari
Don't use direct executor with MockZooKeeper since it could cause ord…
d60b9bc 
…ering problems
@lhotari
Ensure that connection strings don't change during a single execution
2298de6
@lhotari
Improve checkContainers so that it's only called for ZK
0f5c46f
@lhotari
Skip testDeleteUnusedDirectories for MockZooKeeper
701dfad
@lhotari
Improve assertions in getChildren test
13ac174
@lhotari
Fix MockZooKeeper's getChildren implementation
368993b
@lhotari
Consistently unlock before calling callbacks
ea760b4
@lhotari
Restore separate mockZooKeeperGlobal instance
4989e21
@lhotari lhotari force-pushed the lh-fix-ephemeral-support-in-MockZooKeeper branch from 55f62ad to b4e04d5 Compare February 17, 2025 12:11
@lhotari
Properly implement reusing TestZKServer in another context
2c7b042 
- ensures that it has a separate session
@lhotari lhotari force-pushed the lh-fix-ephemeral-support-in-MockZooKeeper branch from b4e04d5 to 2c7b042 Compare February 17, 2025 12:22
@lhotari lhotari merged commit df51972 into apache:master Feb 17, 2025
51 of 52 checks passed
lhotari added a commit that referenced this pull request Feb 17, 2025
@lhotari
[fix][meta] Fix ephemeral handling of ZK nodes and fix MockZooKeeper …
cd1e9db 
…ephemeral and ZK stat handling (#23988)

(cherry picked from commit df51972)
@lhotari
Copy link
Member Author

lhotari commented Feb 17, 2025

cherry-picking depends on #23686

lhotari added a commit that referenced this pull request Feb 17, 2025
@lhotari
[fix][meta] Fix ephemeral handling of ZK nodes and fix MockZooKeeper …
455074b 
…ephemeral and ZK stat handling (#23988)

(cherry picked from commit df51972)
lhotari added a commit that referenced this pull request Feb 17, 2025
@lhotari
[fix][meta] Fix ephemeral handling of ZK nodes and fix MockZooKeeper …
67ae209 
…ephemeral and ZK stat handling (#23988)

(cherry picked from commit df51972)
nikhil-ctds pushed a commit to datastax/pulsar that referenced this pull request Feb 19, 2025
@lhotari @nikhil-ctds
[fix][meta] Fix ephemeral handling of ZK nodes and fix MockZooKeeper …
c4ad461 
…ephemeral and ZK stat handling (apache#23988)

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

@heesung-sn heesung-sn heesung-sn approved these changes

@merlimat merlimat Awaiting requested review from merlimat

@Technoboy- Technoboy- Awaiting requested review from Technoboy-

@nodece nodece Awaiting requested review from nodece

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

@poorbarcode poorbarcode Awaiting requested review from poorbarcode

@shibd shibd Awaiting requested review from shibd

Assignees
No one assigned
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/blocker Indicate the PR or issue that should block the release until it gets resolved release/3.0.10 release/3.3.5 release/4.0.3
Projects
None yet
Milestone
4.1.0
Development

Successfully merging this pull request may close these issues.
2 participants
@lhotari @heesung-sn