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

[Gateway] Remove redundant list kernels request during session poll #1112

Conversation

kevin-bates
Copy link
Member

@kevin-bates kevin-bates commented Dec 2, 2022

Lab services issues /api/kernels and /api/sessions requests every 10 seconds on behalf of its KernelManager and SessionManager instances, respectively. When a gateway server is configured, the /api/kernels request is forwarded to the gateway server, while the /api/sessions request only needs to ensure the kernel model in its list of sessions is still valid (i.e., not culled) - which leads to an additional (but specific) /api/kernels/<kernel_id> request.

In highly latent and multi-tenant installations (in which a single gateway is servicing multiple clients) each of these requests adds up. This pull request eliminates the /api/kernels/<kernel_id> request performed on behalf of the client's /api/sessions request by treating the set of kernel models maintained via the /api/kernels request as the truth. Since the /api/kernels request will detect kernels culled on the gateway server and update its list accordingly, we can use the kernel_id's absence in the list as an indicator it has been culled, triggering the session's deletion from the session table. The net effect of this is that the periodic (and constant) /api/kernels requests are reduced by 50%.

@codecov
Copy link

codecov bot commented Dec 2, 2022

Codecov Report

Base: 80.03% // Head: 80.06% // Increases project coverage by +0.03% 🎉

Coverage data is based on head (8233da1) compared to base (53380a7).
Patch coverage: 100.00% of modified lines in pull request are covered.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1112      +/-   ##
==========================================
+ Coverage   80.03%   80.06%   +0.03%     
==========================================
  Files          68       68              
  Lines        8013     8011       -2     
  Branches     1586     1586              
==========================================
+ Hits         6413     6414       +1     
+ Misses       1179     1177       -2     
+ Partials      421      420       -1     
Impacted Files Coverage Δ
jupyter_server/gateway/managers.py 82.68% <100.00%> (-0.09%) ⬇️
jupyter_server/serverapp.py 80.30% <0.00%> (+0.17%) ⬆️
jupyter_server/services/sessions/handlers.py 80.99% <0.00%> (+0.82%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@kevin-bates
Copy link
Member Author

@blink1073 - I'm fine deferring this merge until post 2.0, unless there's an issue that warrants another RC cycle. Once merged, it would be awesome if we could backport this to 1.x. Thanks.

Copy link
Contributor

@blink1073 blink1073 left a comment

Choose a reason for hiding this comment

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

Thanks! Yeah, I'm happy to merge and backport this after 2.0 final.

@blink1073 blink1073 merged commit ed99ddc into jupyter-server:main Dec 7, 2022
@kevin-bates kevin-bates deleted the gateway-remove-redundant-list-kernels branch February 28, 2023 15:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants