-
Notifications
You must be signed in to change notification settings - Fork 222
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
Zmq port setting #698
Zmq port setting #698
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is great - thank you! I just had a comment relative to naming and units.
docs/source/config-options.md
Outdated
Default: 30000 | ||
Specifies the value of ws_ping_interval that is being used for websocket | ||
ping pong mechanism in ZMQ Port Handler from notebook server. | ||
(NOTEBOOK_ZMQ_PORT_PING_INTERVAL env var) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd prefer we follow the naming (and units) convention that @esevan introduced in jupyter/nb2kg#29. Let's call the option ws_ping_interval
(with the help string indicating seconds) and the env variable EG_WS_PING_INTERVAL_SECS
(defaulting to 30), then expand to ms
on usage.
I don't think we need to mention ZMQ since that corresponds more closely to the (multiple) ZMQ ports - we're really pinging the (single) WS port.
…tead of milliseconds
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changes made according to comments.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM - thank you!
-- Adding option to set ws_ping_interval variable for notebook server usages.