Skip to content

PersistentTTLNode Thread leak#1264

Merged
kezhuw merged 5 commits intoapache:masterfrom
chevaris:ISSUE-1263
Apr 21, 2025
Merged

PersistentTTLNode Thread leak#1264
kezhuw merged 5 commits intoapache:masterfrom
chevaris:ISSUE-1263

Conversation

@chevaris
Copy link
Copy Markdown
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
Copy Markdown
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.

Comment thread ...-recipes/src/test/java/org/apache/curator/framework/recipes/nodes/TestPersistentTtlNode.java Outdated
Comment thread ...-recipes/src/test/java/org/apache/curator/framework/recipes/nodes/TestPersistentTtlNode.java Outdated
Comment thread ...-recipes/src/test/java/org/apache/curator/framework/recipes/nodes/TestPersistentTtlNode.java Outdated
Comment thread ...-recipes/src/test/java/org/apache/curator/framework/recipes/nodes/TestPersistentTtlNode.java Outdated
Comment thread ...-recipes/src/test/java/org/apache/curator/framework/recipes/nodes/TestPersistentTtlNode.java Outdated
kezhuw
kezhuw approved these changes Apr 17, 2025
View reviewed changes
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Contributor Author

chevaris commented Apr 18, 2025

I will provide a fix. Thanks for the heads up

@chevaris
Copy link
Copy Markdown
Contributor Author

chevaris commented Apr 21, 2025

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
Copy Markdown
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