AllExcept for Groups - GroupExcept #1204
Conversation
analogrelay
left a comment
analogrelay
left a comment
There was a problem hiding this comment.
Looks good to me so far.
| throw new ArgumentNullException(nameof(groupName)); | ||
| } | ||
|
|
||
| var message = new RedisExcludeClientsMessage(GetInvocationId(), nonBlocking: true, target: methodName, excludedIds: excludedIds, arguments: args); |
There was a problem hiding this comment.
I like this. It uses an existing message and we just publish it to a different Redis key. 👍
Story of my week ;). We need to get a handle on hanging and flaky tests. There are a couple of options to help diagnose them. Once #1147 is merged we'll get long-running test notifications which will tell you which tests are still running after 5 seconds. |
mikaelm12
force-pushed
the
mikaelm12/GroupExcept
branch
from
2193580 to
f803e6d
Compare
|
Fixed the hang so I need to add test now 😄 |
| var tasks = group.Values.Where(connection => !excludedIds.Contains(connection.ConnectionId)) | ||
| .Select(c => c.WriteAsync(message)); | ||
| return Task.WhenAll(tasks); | ||
| } |
There was a problem hiding this comment.
Is this a reasonable place to add logging or should we first finalize our logging strategy? @BrennanConroy @anurse
mikaelm12
changed the title
|
I have a growing concern that we should look at addressing and it's about the explosion of methods on the HubLifetimeManager as a result of these slices of connections. I want us to investigate a named filter concept to see if we can avoid some of the explosion. |
|
@davidfowl I had the same thoughts. I think we can get away with a single Group primitive and write the others like that (including All). |
mikaelm12
force-pushed
the
mikaelm12/GroupExcept
branch
from
ca7c7d6 to
e87c987
Compare
| { | ||
| private readonly string _groupName; | ||
| private readonly HubLifetimeManager<THub> _lifetimeManager; | ||
| private IReadOnlyList<string> _excludedIds; |
| await manager.AddGroupAsync(connection1.ConnectionId, "gunit").OrTimeout(); | ||
| await manager.AddGroupAsync(connection2.ConnectionId, "gunit").OrTimeout(); | ||
|
|
||
| var excludedIds = new List<string>{client2.Connection.ConnectionId }; |
|
|
||
| await connection1.DisposeAsync().OrTimeout(); | ||
| await connection2.DisposeAsync().OrTimeout(); | ||
| Assert.Null(client2.TryRead()); |
This is in preparation for
OthersInGroup.GroupExceptis analogous toAllExceptso that we can makeOthersInGroupwork similarly to OthersStill working on this but was looking for early feedback. Hanging some tests btw... any insight there would be appreciated.
Haven't added new tests for this feature yet as well.