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

fix: replace stream connection API key from kernel id to session id #1622

Draft
wants to merge 13 commits into
base: main
Choose a base branch
from

Conversation

fregataa
Copy link
Member

@fregataa fregataa commented Oct 16, 2023

follow-up #576

Change a key of session connection info from session_id to kernel_id

Checklist: (if applicable)

  • Milestone metadata specifying the target backport version

@fregataa fregataa added the urgency:4 As soon as feasible, implementation is essential. label Oct 16, 2023
@fregataa fregataa added this to the 23.03 milestone Oct 16, 2023
@fregataa fregataa self-assigned this Oct 16, 2023
@github-actions github-actions bot added comp:manager Related to Manager component size:S 10~30 LoC labels Oct 16, 2023
@fregataa fregataa requested a review from achimnol October 16, 2023 05:07
- It is better to cast the ID columns using explicit constructor
  of the subtype.

- The stream key is now based on session IDs, but still the callback's
  partial arguments are only kernel IDs. This may confuse future code
  readers.

- As of Python 3.8, `functools.partial()` now has correct type
  inference and support for async functions.
@github-actions github-actions bot added size:M 30~100 LoC and removed size:S 10~30 LoC labels Oct 16, 2023
Copy link
Member

@achimnol achimnol left a comment

Choose a reason for hiding this comment

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

I've done a little refactoring. Added another review to let you look once more.

Comment on lines 469 to 472
stream_key = session_id
stream_id = uuid.uuid4().hex
app_ctx.stream_proxy_handlers[stream_key].add(myself)
defer(lambda: app_ctx.stream_proxy_handlers[stream_key].discard(myself))
app_ctx.stream_proxy_handlers[kernel_id].add(myself)
defer(lambda: app_ctx.stream_proxy_handlers[kernel_id].discard(myself))
Copy link
Member

Choose a reason for hiding this comment

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

stream_proxy_handlers in other places (e.g., handle_kernel_terminating()) still uses kernel.id as the stream key. We should make the naming of stream_key consistent in two different usage scenarios in this module.

@github-actions github-actions bot added size:L 100~500 LoC and removed size:M 30~100 LoC labels Oct 16, 2023
@github-actions github-actions bot added size:XL 500~ LoC and removed size:L 100~500 LoC labels Oct 17, 2023
@fregataa fregataa requested a review from achimnol October 23, 2023 13:48
@fregataa fregataa marked this pull request as draft February 29, 2024 06:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
comp:manager Related to Manager component size:XL 500~ LoC urgency:4 As soon as feasible, implementation is essential.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants