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

A channel pool #3310

Merged
merged 1 commit into from
Oct 4, 2024
Merged

A channel pool #3310

merged 1 commit into from
Oct 4, 2024

Conversation

steveluscher
Copy link
Collaborator

Summary

This is a special RpcSubscriptionsChannelCreator that creates a pool of underlying channels up to minChannels, reuses them evenly up to maxSubscriptionsPerChannel, and tears the channels down when all of its subscribers abort.

Copy link

changeset-bot bot commented Oct 4, 2024

⚠️ No Changeset found

Latest commit: 8e4edda

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

Comment on lines +30 to +32
// Start from the item two positions after the current item. This way, the
// search will finish on the item after the current one. This ensures that, if
// any channels tie for having the most capacity, the one that will be chosen is
// the one immediately to the current one's right (wrapping around).
Copy link
Collaborator

Choose a reason for hiding this comment

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

Correct me if I'm wrong but I think this is only true if the channel immediately to the current one's right is part of the tie.

Consider the following channel scenarios:

  • A(1), B(1), C(1), D(1) where A is the current one. Then here B is the next one because they are all tied.
  • A(5), B(5), C(1), D(1) where A is the current one. Then here D is the next one whereas we were probably expecting C as per the rotation principle.

Wouldn't it be better to just do ii + 1 but then have a strict greater than in the condition below (i.e. mostFreeChannel.subscriptionCount > nextPoolEntry.subscriptionCount)? That way, we only override if the next one is better (i.e. less subscribed to).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It's pretty insane, but in this case:

A(5), B(5), C(1), D(1) where A is the current one. Then here D is the next one whereas we were probably expecting C as per the rotation principle.

A can't be the current one, because the next-free-channel is chosen ahead of time. The next free channel would always be C or D depending on where we were in the rotation.

Next free channels are chosen:

  • as a last step, after one is added.
  • as a last step, after one is removed.

The next time that a channel is added, it goes right where the freeChannelIndex points, no questions asked.

I think covered all the weird cases in the tests, but let me know if you can think of one that breaks things!

Copy link
Collaborator

Choose a reason for hiding this comment

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

If I understand correctly, the second scenario cannot happen due to how this function is being used within the decorator.

I think that's fine since we've got good test coverage but wouldn't it be better for maintainability if this function alone was responsible for keeping all the invariants of finding the next channel regardless of how it is being called?

@@ -18,6 +18,16 @@ This package contains types that implement RPC subscriptions as required by the

## Functions

### `getChannelPoolingChannelCreator({ createChannel, maxSubscriptionsPerChannel, minChannels })`
Copy link
Collaborator

Choose a reason for hiding this comment

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

Since this is essentially a channel creator decorator, how would you feel about making the createChannel attribute a proper argument so it can be piped more easily?

Something like:

const channelCreator = pipe(
  getMyOriginalChannelCreator(),
  cc => getChannelPoolingChannelCreator(cc, { ... }),
)

@steveluscher steveluscher force-pushed the 10-02-a_channel_augmenter_that_encodes_and_decodes_messages_as_json branch from fd5fc49 to 1ac504b Compare October 4, 2024 17:56
@steveluscher steveluscher force-pushed the 10-02-a_channel_augmenter_that_encodes_and_decodes_messages_as_json branch 2 times, most recently from a721e9a to 2883f22 Compare October 4, 2024 18:05
@steveluscher steveluscher force-pushed the 10-02-a_channel_augmenter_that_encodes_and_decodes_messages_as_json branch from 2883f22 to 3007e83 Compare October 4, 2024 19:25
@steveluscher steveluscher force-pushed the 10-02-a_channel_augmenter_that_encodes_and_decodes_messages_as_json branch from 3007e83 to 12931ce Compare October 4, 2024 19:26
@steveluscher steveluscher changed the base branch from 10-02-a_channel_augmenter_that_encodes_and_decodes_messages_as_json to graphite-base/3310 October 4, 2024 19:29
@steveluscher steveluscher changed the base branch from graphite-base/3310 to master October 4, 2024 19:32
Copy link
Collaborator Author

steveluscher commented Oct 4, 2024

Merge activity

@steveluscher steveluscher merged commit e1fa14b into master Oct 4, 2024
9 checks passed
@steveluscher steveluscher deleted the 10-02-a_channel_pool branch October 4, 2024 21:09
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.

2 participants