Skip to content
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

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

simeonschaub
Copy link
Member

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.

@simeonschaub
Copy link
Member Author

@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?

@jpsamaroo
Copy link
Member

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.

@simeonschaub
Copy link
Member Author

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.

@vchuravy
Copy link
Member

vchuravy commented Nov 2, 2023

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.

simeonschaub and others added 2 commits September 6, 2024 14:04
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.
Copy link

codecov bot commented Sep 6, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 79.34%. Comparing base (af89e6c) to head (dcc7da8).

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.
📢 Have feedback on the report? Share it here.

@simeonschaub
Copy link
Member Author

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?

@simeonschaub
Copy link
Member Author

@vchuravy or @jpsamaroo would you mind taking another look at this?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants