-
Notifications
You must be signed in to change notification settings - Fork 242
Create a custom ContainerManager for purging the TTL based znodes #3038
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
base: master
Are you sure you want to change the base?
Conversation
zookeeper-api/src/test/java/org/apache/helix/zookeeper/impl/ZkTestBase.java
Show resolved
Hide resolved
.../src/test/java/org/apache/helix/zookeeper/impl/client/RealmAwareZkClientFactoryTestBase.java
Show resolved
Hide resolved
junkaixue
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a good start of the work! Make sure not breaking the test. But happy to see the tests to be stablized later.
b6c607b to
53f4ef2
Compare
53f4ef2 to
f6ad476
Compare
Hi @junkaixue , fixed the test issues, triggered CI through my fork, it seems to be passing now: https://github.com/LZD-PratyushBhatt/helix/actions/runs/15034195323/job/42252802394?pr=1 |
| public ZooKeeperServer getZooKeeperServer() { | ||
| return _zk; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you using Helix style to format it? It looked wired. For the place you touched, we should fix it. Maybe it was not handled before.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @junkaixue , What do we mean by "helix style" here? I had earlier formatted the method in same way just as the getZkClient method above this.
| /** | ||
| * Creates a ContainerManager with custom elapsed time functionality for a ZkServer | ||
| */ | ||
| private static ContainerManager createContainerManager(ZkServer zkServer) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This function has been repeated. Better make it as Util or in base class.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One is in helix-core module and other is in zookeeper-api. And in zookeeper-api I dont see any packaging of test classes being done. So if we make this common then we will need to modify pom.xml in some places.
Issues
(#200 - Link your issue number here: You can write "Fixes #XXX". Please use the proper keyword so that the issue gets closed automatically. See https://docs.github.com/en/github/managing-your-work-on-github/linking-a-pull-request-to-an-issue
Any of the following keywords can be used: close, closes, closed, fix, fixes, fixed, resolve, resolves, resolved)
This PR fixes #3036 TTL Znode doesn't expire in Test envs
Description
(Write a concise description including what, why, how)
The issue is related to ZooKeeper's TTL (Time-To-Live) node functionality and specifically how it behaves in the embedded ZooKeeper environment used in tests.
In a normal ZooKeeper deployment, a background ContainerManager thread runs periodically to clean up expired TTL nodes. However, in the embedded ZooKeeper used for testing, this background thread doesn't appear to be functioning properly. The ContainerManager is designed to run on a configurable schedule (default is 60 seconds), but in the test environment, this cleanup process doesn't happen.
The way we solve it is by creating a custom ContainerManager and using a fakeElapsedTimer.
Tests
No new tests, but old tests modified.
(List the names of added unit/integration tests)
(If CI test fails due to known issue, please specify the issue and test PR locally. Then copy & paste the result of "mvn test" to here.)
Changes that Break Backward Compatibility (Optional)
(Consider including all behavior changes for public methods or API. Also include these changes in merge description so that other developers are aware of these changes. This allows them to make relevant code changes in feature branches accounting for the new method/API behavior.)
Documentation (Optional)
(Link the GitHub wiki you added)
Commits
Code Quality
(helix-style-intellij.xml if IntelliJ IDE is used)