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

#34302 add ipv6 square bracket support to launch_on_machine #34430

Closed
wants to merge 0 commits into from
Closed

#34302 add ipv6 square bracket support to launch_on_machine #34430

wants to merge 0 commits into from

Conversation

ssikdar1
Copy link
Contributor

@ssikdar1 ssikdar1 commented Jan 18, 2020

PR for #34302 to let launch_on_machine handle ipv6 address by adding support for the square bracket notation.

Note: this does not address the ipv6 socket related problems reported in the issue.

  • add a function to handle both ipv4 and ipv6 host strings
  • Add a few tests.
  • Add a NEWS item.

Testing:
I added a unit test julia file. It includes managers.jl directly and tests some simple use cases.
127.0.0.1 , 127.0.0.1:80, [2001:db8::1], [2001:db8::1]:443

I also ran $ ./julia stdlib/Distributed/test/runtests.jl with no issues.

@Keno
Copy link
Member

Keno commented Jan 19, 2020

Please rebase this branch to only have the relevant commit.

@ssikdar1
Copy link
Contributor Author

@Keno sorry about that! branch rebased

@StefanKarpinski
Copy link
Member

Going a bit further, since you're introducing the parse_machine function, I would suggest that it should parse the port number all the way to validated port integer or nothing in the parse function rather than right afterwards. I was also going to suggest parsing the host part to an IPAddr (IPv4 or IPv6 instance) but it could also be a hostname, so there would be a lot of potential return types, which is not actually a big problem, but then it gets immediately turned back into a string and passing through the IPAddr type could lead to differences in formatting which might just confuse matters. So probably best to push the port validation into the parse_machine function but return the host as a string.

@ssikdar1
Copy link
Contributor Author

So probably best to push the port validation into the parse_machine function but return the host as a string.

8b9bbb1 : Pushed the port validation to inside parse_machine , parse_machine still returns host as a string. Added some unit tests to test the port edge cases

@StefanKarpinski
Copy link
Member

One simplifying comment but otherwise looks good!

@StefanKarpinski
Copy link
Member

Ok. This is looking great. Now all you need is a NEWS entry! It should probably go in the standard library changes section under a "Distributed" subheading.

@StefanKarpinski
Copy link
Member

This is good to go when CI passes. Whoever merges, please squash!

@@ -0,0 +1,19 @@
using Test
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test doesn't run unless it's added to runtests.jl

@ssikdar1 ssikdar1 closed this Jan 23, 2020
@StefanKarpinski
Copy link
Member

Did you mean to close this? We're so close to being done here...

@ssikdar1
Copy link
Contributor Author

ssikdar1 commented Jan 24, 2020

sorry!! I had done a reset and force push without thinking, I must have closed the issue then by accident. I restored my fork to the previous commit and added a change for runtests, but seem unable to click the "reopen pull request" on github.

I opened a new PR here: #34494

@StefanKarpinski
Copy link
Member

I can't reopen it either. Guess you'll need to make a new PR.

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.

4 participants