-
Notifications
You must be signed in to change notification settings - Fork 387
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
feat(core): Add support for Container and TTL nodes #613
base: master
Are you sure you want to change the base?
Conversation
4634c04
to
3685ec2
Compare
3685ec2
to
f9b4498
Compare
571168e
to
696e51e
Compare
Still missing tests. Will need to update the harness to configure Zookeeper to enable extended types. |
@ceache just curious on the status here? I assume you've just been busy? Or have you abandoned this for some reason and forgot to close it? |
Well, I have been incredibly busy, sorry about that. But not idle.
I have implemented a Wireshark zookeeper protocol dissector (I'll link it
here) and confirmed the issue is indeed in zookeeper.
https://issues.apache.org/jira/browse/ZOOKEEPER-4026
The PR is otherwise not wrong, albeit it needs a little tidy up before
merge.
But I am not sure how to go about testing it. (be permissive?)
…On Sun, Dec 13, 2020, 15:31 Jeff Widman ***@***.***> wrote:
@ceache <https://github.com/ceache> just curious on the status here? I
assume you've just been busy? Or have you abandoned this for some reason
and forgot to close it?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#613 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAIFTHXSXLUP4ZCHTQODJDLSUUQAPANCNFSM4N7HORNQ>
.
|
Haha, I also have been working on a Wireshark zookeeper dissector the last few months in my spare time as a way to learn C/wiresharp APIs/understand the ZK protocol at a deeper level: https://gitlab.com/wireshark/wireshark/-/merge_requests/528 I've had to put it on hold the last few weeks as we somewhat spontaneously decided to buy a house so I've been busy with that, but hope to pick it back up during my time off over the holidays. I would say for testing, being permissive is fine with me. I'm also completely fine with letting this sit for a bit longer to get a ZK release with the bugfix if that makes testing this simpler... whatever you prefer/think is wisest. It would be nice to have a test, but I defer to your judgement. I just wanted a status update since there'd been no update since June. |
Mine is a LUA dissector https://github.com/ceache/zab_dissector |
Hi! In short I've found myself in the need for nodes with TTL. I'm not entirely sure how this PR is blocked, but I figured it might be good enough for me. I've forked, merged it in and added a simple test to check if persistent TTL nodes behave as expected. They work great as far as I can tell. You can find it here: https://github.com/Traktormaster/kazoo/tree/ttlme I'm not sure how to offer that commit into this PR myself, but you may find it useful when getting back to work on this. Thanks! |
95b7cd5
to
2bb9471
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #613 +/- ##
==========================================
- Coverage 96.81% 93.58% -3.24%
==========================================
Files 27 57 +30
Lines 3549 8418 +4869
==========================================
+ Hits 3436 7878 +4442
- Misses 113 540 +427 ☔ View full report in Codecov by Sentry. |
26a151c
to
b9c6d51
Compare
fb9c9dc
to
84a0024
Compare
84a0024
to
bf29a6f
Compare
bf29a6f
to
6b83f17
Compare
6b83f17
to
69a3271
Compare
Hello, is there any updates on the release of this PR? |
69a3271
to
a7b7c25
Compare
476d616
to
b030864
Compare
Also add support through transactions. Closes python-zk#334, python-zk#496
b030864
to
db886ae
Compare
Also add support through transations.
Closes #334, #496