You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@curator.apache.org by gtully <gi...@git.apache.org> on 2016/08/18 18:39:46 UTC
[GitHub] curator pull request #161: CURATOR-344 - limit shared value watcher to valid...
GitHub user gtully opened a pull request:
https://github.com/apache/curator/pull/161
CURATOR-344 - limit shared value watcher to valid change events to av\u2026
\u2026oid work on disconnect.
Test that shows the difficulty with blocking the event thread.
When the framework is shared and one use case needs timely notification of suspend, a use case that requires retry for some period gets in the way.
In any event, on a disconnect, there may or may not be change, it does not make sense to me that the listeners should be fired.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/gtully/curator CURATOR-344
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/curator/pull/161.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #161
----
commit f89ddfafb45c4656ea34474f061ca902c7060116
Author: gtully <ga...@gmail.com>
Date: 2016-08-18T18:34:10Z
CURATOR-344 - limit shared value watcher to valid change events to avoid work on disconnect
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] curator issue #161: CURATOR-344 - limit shared value watcher to valid change...
Posted by cammckenzie <gi...@git.apache.org>.
Github user cammckenzie commented on the issue:
https://github.com/apache/curator/pull/161
I'm not sure on the pattern for executors, probably just the whim of whoever implemented the recipe I imagine.
I think that you should be able to do this asynchronously without an executor though. If you change the readValue() method to have an option of executing in the background then it should fix the problem I think. When being called from the Watcher, you would make the readValue getData() call in the background and update the shared value in the asynchronous callback.
That should fix the problem properly and not require the introduction of an executor. I think that we should clean up the events which actually result in the readValue() being called from the Watcher too.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] curator issue #161: CURATOR-344 - limit shared value watcher to valid change...
Posted by gtully <gi...@git.apache.org>.
Github user gtully commented on the issue:
https://github.com/apache/curator/pull/161
Thanks for taking the time to peek at this. I agree.
Doing everything async will make it a little heaver weight but I think it is more correct.
There seems to be be some recipes that take executors, others that don't, but I have not figured out if there is some pattern to this. Do you know?
Short term, I can fix up the unit test to be deterministic (i.e. wait for connected).
Maybe this can be tackled in two phases. The first making the watcher more discriminating. For my use case, disconnecting is the important event.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] curator issue #161: CURATOR-344 - limit shared value watcher to valid change...
Posted by cammckenzie <gi...@git.apache.org>.
Github user cammckenzie commented on the issue:
https://github.com/apache/curator/pull/161
I don't think that this completely fixes the problem. The test fails intermittently because it's possible that the server gets stopped while the SharedValue is processing the initial connected event. When this happens, it goes through the retry loop and the assertion at the end of the test fails.
I think that ultimately the processing that occurs within the watcher's process() method should be run asynchronously, and equally, the watcher should be more discriminant about which events it's interested in.
Thoughts?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] curator issue #161: CURATOR-344 - limit shared value watcher to valid change...
Posted by gtully <gi...@git.apache.org>.
Github user gtully commented on the issue:
https://github.com/apache/curator/pull/161
thanks for the feedback.
I fixed up the unit test to make it more deterministic.
Changed the watcher to do an update and notification in the background.
That sorts my use case however the update events should only fire on valid change events.
Added additional unit test to track change events between disconnect and reconnect and made the matching change to restrict the callback to valid change events.
I think it is better on both counts.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] curator issue #161: CURATOR-344 - limit shared value watcher to valid change...
Posted by cammckenzie <gi...@git.apache.org>.
Github user cammckenzie commented on the issue:
https://github.com/apache/curator/pull/161
Looks good to me, thanks for the patch!
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] curator pull request #161: CURATOR-344 - limit shared value watcher to valid...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/curator/pull/161
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---