-
Notifications
You must be signed in to change notification settings - Fork 11
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
allow initiating peer to close stream gracefully #9
base: master
Are you sure you want to change the base?
Conversation
10b6e60
to
8923a40
Compare
@jpsamaroo Looks like GitHub won't let me request a review, but I'd be curious to hear your thoughts on this. Is there a reason we might still want a closed connection to be fatal here? |
This seems like a good idea to me, but I will yield to @vchuravy for further review. Until then, I'd want a test to ensure that the worker only remains alive when it's incoming to the head node. Additionally, some documentation on how to initiate this kind of connection (with an example) would be an excellent addition to the docs. |
Agree! My current thinking is that it might be best to move this functionality into a new package and then just link to that in the documentation. Since Distributed is just a regular package now, we should be able to then add this as a test dependency here, right? Eventually, it might even make sense to move the implementation here but I'd like to experiment with this some more first before we commit to an official API. |
We certainly would need a test here for this. Right now I am moving slowly on Distributed.jl since I have my hands full with the rest of the stdlibs and I want to move cautiously until we have 1.11 registered. |
I have a usecase where I would like to remove a worker without killing it. Currently, trying to disconnect from the head node will cause the message handler loop to throw a fatal exception, so this adds a check that the connection is still open when trying to read new messages.
8923a40
to
3a4bdf4
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #9 +/- ##
==========================================
+ Coverage 79.14% 79.34% +0.19%
==========================================
Files 10 10
Lines 1899 1898 -1
==========================================
+ Hits 1503 1506 +3
+ Misses 396 392 -4 ☔ View full report in Codecov by Sentry. |
I have now added an integration test with a lot of the code from https://github.com/simeonschaub/PersistentWorkers.jl. I couldn't really think of a better way to test this. What do you think? |
@vchuravy or @jpsamaroo would you mind taking another look at this? |
I have a usecase where I would like to remove a worker without killing it. Currently, trying to disconnect from the head node will cause the message handler loop to throw a fatal exception, so this adds a check that the connection is still open when trying to read new messages.
A minimal example of what I'm trying to do can be found here, could adapt that into a test if so desired.