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

Clarify bind and connect addresses throughout JupyterHub #1289

Closed
yuvipanda opened this issue Jul 28, 2017 · 6 comments · Fixed by #1850
Closed

Clarify bind and connect addresses throughout JupyterHub #1289

yuvipanda opened this issue Jul 28, 2017 · 6 comments · Fixed by #1850

Comments

@yuvipanda
Copy link
Contributor

Currently we use ip, port, connect_ip, connect_port for various things. It's a little confusing, and not very clear what is what when reading code.

I propose for 0.9, we do the following:

  1. Deprecate ip and port, connect_ip and connect_port. They'll still work
  2. Turn ip and port into bind_address, which can be ip:port combo, unix domain socket, just ip, etc.
  3. Turn connect_ip and connect_port into connect_address, which would do the same
  4. Use this across the JupyterHub codebase, clarifying for Proxies and Singleuser servers as well.
@willingc
Copy link
Contributor

+1

@Carreau
Copy link
Member

Carreau commented Jul 28, 2017

I completely agree with that, I just didn't wanted to implement that for 0.8 with all the already existing changes. I would like to push for a non-py config files as default (traitlets works perfectly with Json), that would allow to have a command to upgrade config files.

@yuvipanda
Copy link
Contributor Author

Yeah, I agree we shouldn't do this for 0.8. We don't really need to support unix domain sockets for 0.8 tho - our performance bottlenecks are elsewhere still (mostly in sqlalchemy blocking main thread now, I think).

@willingc willingc added this to the 0.9 milestone Jul 28, 2017
@willingc
Copy link
Contributor

Adding to the 0.9 milestone.

@minrk
Copy link
Member

minrk commented Jul 28, 2017

Since connect_ip and port will be new in 0.8, maybe we should drop those in favor of connect_address now, and add bind_address as a new concept in 0.9?

@minrk
Copy link
Member

minrk commented Jul 28, 2017

On second thought, I think connect_ip is a special case - Hub listening on all interfaces by having to specify a connect IP is common to all distributed deployments (even local docker), which is what prompted bringing it upstream out of DockerSpawner. Specifying anything else (connect_port, otherwise different full URL) is on a quite different level. So I'm inclined to keep connect_ip as it is now, and maybe trade only connect_port for a full connect_url (should include http[s]://), since that's brand new and only for a pretty advanced use case.

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

Successfully merging a pull request may close this issue.

4 participants