Skip to content

PersistentTTLNode Thread leak #1264

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
merged 5 commits into from
Apr 21, 2025
Merged

PersistentTTLNode Thread leak #1264

merged 5 commits into from
Apr 21, 2025

Conversation

chevaris
Copy link
Contributor

@chevaris chevaris commented Apr 8, 2025

@chevaris chevaris mentioned this pull request Apr 8, 2025
Closed
kezhuw
kezhuw reviewed Apr 16, 2025
View reviewed changes
Copy link
Member

@kezhuw kezhuw left a comment

Choose a reason for hiding this comment

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

@chevaris Thank you for contribution! Looks good in general. Left some inline comments in reviewing.

kezhuw
kezhuw approved these changes Apr 17, 2025
View reviewed changes
Copy link
Member

@kezhuw kezhuw left a comment

Choose a reason for hiding this comment

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

LGTM

@kezhuw
Copy link
Member

kezhuw commented Apr 18, 2025

Seems that the test fails due to ZOOKEEPER-4466 which is only shipped in 3.9. Both PersistentNode and PersistentWatcher(from test code) set watches(one exist and persistent_recursive) on "/parent/main". I think we probably should refactor test code to watch on "/parent", otherwise we might have to excludes these two tests from zk38 and zk37.

For the same sake, I think testTouchNodeNotCreated is probably fragile to timing.

@chevaris
Copy link
Contributor Author

I will provide a fix. Thanks for the heads up

@chevaris
Copy link
Contributor Author

I see that zk38 is failing BUT is not related with the changes. TesPersistentTtlNode are passing properly. Not sure how to proceed

@kezhuw kezhuw merged commit 57cf513 into apache:master Apr 21, 2025
10 checks passed
@kezhuw
Copy link
Member

kezhuw commented Apr 21, 2025

Merged. @chevaris Thank you for your contribution!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kezhuw kezhuw kezhuw approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@chevaris @kezhuw