-
Notifications
You must be signed in to change notification settings - Fork 55
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
Experimental v2/channel and consumer group #1661
base: main
Are you sure you want to change the base?
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.
Implementation looks good, I am happy to release with experiment tag
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 overall, small comment about polyfill.
Test checks are looking weird though:
All integration tests are failing with an abnormal time (100m - 300m) - can't even load logs from them, there are just so many after running for so long.
Maybe there is a flaky/failed test that is just stuck in a loop or doesn't have a timeout set or something.
Should probably make sure tests are passing before releasing
@VeskeR I pushed up some changes to tests. They pass reliably for me locally so not sure why we're getting issues in CI. Will wait for the run to finish and inspect the logs |
Still failed after 100+ minutes unfortunately. Hope you will be able to load some logs, there are a lot. Quickly looked at previous PRs: PRs before that had their tests passing in CI. For example, #1651 precedes the PR above and its tests either pass, or fail in a reasonable time (~10 minutes) |
From the logs, there's a lot of
So I have bumped the default connection limits for the test account from 100 to 200, and will see what effect that has. |
Actually, looks like there's a lot of
Which is causing a large number of connections to be terminated and reopened. Looking into it |
Add a client side channel group that listens for active channels and subscribes/unsubscribes as the set of active changes. Add a consumer group based on presence set Add typing information for ChannelGroups Fix tests for v2 ChannelGroups is a default realtime module types: add channel group typings consumergroup: use modulo-based hashing scheme The `hashring` package is node-only as it depends on the native `crypto` package. Replaced with a simple modulo hash scheme for now. Fixes the case where the channel is already attached and the channel is obtained with new rewind options that require a re-attach. Updates the consumer group partitioning test to more robustly assert that channels are partitioned across consumers. channelgroup: make get sync, subscribe async This matches the pattern used by Channels, which is sync to obtain the channel and async on subscribe in order to await attachment. In the channel group case, awaiting the subscribe awaits the joining of the consumer group. consumergroup: make consumerId required field consumergroup: make hashring a required field format: apply prettier formatting rules jsdoc: add consumer group docs channelgroups: add active channel name option channelgroup: add explicit join method Exposes the join method on the channel group, which is analogous to attach on the channel. Uses this in tests for more robust assertions. Additionally avoids re-attaching to the consumer group channel if already attached, and obtains presence membership synchronously in the join. consumergroup: use subscribe over on The `on` method was not reliable across clients, despite being documented in https://ably.com/docs/presence-occupancy/presence?lang=java#synced, so use subscribe instead. channelgroup: fix assigned channel processing We need to keep the total set of active channels around as updating the assigned channel set when the membership changes requires computing the new assignments from the complete channel set, not the previously set of assigned channels. consumergroup: include consumerId in logs consumergroups: add test for consumer group resize test: remove explicit join from test lint: apply formatting and cleanup test: replace var with let test: remove unnecessary outer try-catch test: add prefix to channels Avoid channel name collisions causing tests to fail from concurrent test runs in CI. channelgroup: detach from channel on un-assignment channelgroup: add unsubscribe listener method test: fix rebalance test waits for consumers channelgroup: add leave method test: remove dangling console logs test: test consumer group scale down event test: prefix consumer group channel Similar to the active channel, we need to avoid conflicts. consumergroup: fix current member tracking We store the current active set of members in the hashring. test: fix done condition w/ at-least-once delivery Messages can be delivered more than once during a consumer group rescaling event, so deduplicate the results when checking the end condition. channelgroups: use Utils.inspectError in logs channelgroups: do not share channel object The Channels object used by the ChannelGroup for internal channel operations no longer shares the same object exposed on the client via the .channels() method. This is to ensure that independent usage of an individual channel that happens to be included in a channel group is not impacted by its usage in the channel group. test: tidy up leave test Now that we can correctly handle a channel group and channel being used independently from the same client, this tidies up the leave test to remove the additional client previously needed. test: rename waitForConsumers for clarity test: rename waitForConsumers for clarity channelgroups: do not share channel object The Channels object used by the ChannelGroup for internal channel operations no longer shares the same object exposed on the client via the .channels() method. This is to ensure that independent usage of an individual channel that happens to be included in a channel group is not impacted by its usage in the channel group. channelgroups: add module integration modules: update ChannelGroups module definitions channelgroup: add temp rewind channel group option channelgroup: unsubscribe channel after timeout In order to avoid keeping the channel alive, we add a configurable timeout after which the channel will be unsubscribed if no messages are received. This is to avoid keeping the channel active. This can lead to missed messages if the a message is published after the client unsubscribes and before the channel becomes inactive. This is an acceptable edge case for the client-side simulation, especially with the default 1h timeout. deps: remove unused hashring types pkg utils: remove arrIndexOf polyfill consumergroup: rename hashring to locator test: use async style tests for channel groups Replaces the use of the `done()` callback with an async function style test. This allows us to await channel publish results and more easily handle race conditions in tests. channelgroup: use qualifier options Previously we relied on a new BaseRealtime instance with it's own Channels object to separate usage of channels in the ChannelGroup from independent external usage of those channels from the regular client.channels.get() method. This led to various problems with shared Auth state such as nonces in token requests which caused connections to terminate and tests to fail. A simpler solution is to avoid creating a new client instance and instead share the Channel pool, but force the library to treat channels used from the ChannelGroup independently (with their own attachment) by setting dummy options in the qualifier, which is used as the key in the channel map. This implementation does not support channels in the channel group which already have a qualifier. This is acceptable for the experimental client-side simulation of the feature.
10068f9
to
bcf965f
Compare
@ttypic @VeskeR Tests are now passing. The problem was due to some shared auth state causing conflicts between simultaneous usage of the two |
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.
Looks good, left two small comments
@@ -44,6 +44,20 @@ define([ | |||
|
|||
async function monitorConnectionAsync(action, realtime, states) { | |||
const monitoringResultPromise = new Promise((resolve, reject) => { | |||
if (Object.prototype.toString.call(realtime) == '[object Array]') { |
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.
May be Array.isArray(realtime)
here ?
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.
Yes probably better, I was following the pattern used in closeAndFinish
@@ -0,0 +1,701 @@ | |||
'use strict'; |
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 realized that test files are not automatically picked up by the playwright browser tests as they are for node.
If you check, for example, this CI run for webkit in this PR you can see there are no realtime/channelgroup
tests completed.
It seems that we need to add new test file names to test/support/browser_file_list.js
for it to be included in browser tests. I hope it's the only place we need to change for that.
You can check locally if it works if you do describe.only('realtime/channelgroup'
below, and run PLAYWRIGHT_BROWSER=chromium npm run test:playwright
(might need to do npx playwright install --with-deps
before that to install browsers)
Each client will be entered in the presence set once per connection, so we need to deduplicate the presence set by clientId.
…mitter channelgroup: emit channel assigned/active events
…events consumergroup: emit membership updated event
Since the set of active channels are surfaced on $ably:active is a superset of those specified by a particular channel group expression, this commit ensures we only emit an active.updated event when the set of channels matching the consumer group expression have actually changed.
Labelled draft as not intended to merge. We will create a special release tag from this branch for customers requiring preview access to this functionality.
Adds a client-side simulation of channel + consumer groups functionality.
A channel group identifies a set of channels using a regular expression. The client SDK is responsible for subscribing to channels that match the regex as they become active and unsubscribing as they become inactive. Active channels are made known to the client via a dedicated channel named
$ably:active
which for the POC is published to by a lambda integration rule.The set of channels in the channel group can be partitioned across a set of consumers that share the same
consumerGroup.name
. Consumers become aware of one another via presence on a channel with the same name as the consumer group. Channels are partitioned according to a simple modulo hash based partitioning scheme.This implementation is intended to demonstrate the channel/consumer groups functionality and is not intended for usage in production. In a production-ready implementation, channel and consumer groups will be implemented in the backend realtime system and the client implementation will be much simpler. However this PR is indicative of what the public API would look like (except for some clearly-labelled simulation-specific configuration).