[Compatibility] Added CLIENT UNBLOCK command #886
Conversation
…-Nirmal/garnet into new/CLIENT-UNBLOCK-command
| @@ -46,6 +46,11 @@ public class CollectionItemBroker : IDisposable | |||
| private bool disposed = false; | |||
| private bool isStarted = false; | |||
|
|
|||
| internal bool TryGetObserver(int sessionId, out CollectionItemObserver observer) | |||
There was a problem hiding this comment.
Please add an XML comment
| } | ||
| } | ||
|
|
||
| if (Server is GarnetServerBase garnetServer) |
There was a problem hiding this comment.
If this is false you should abort with some error, otherwise the client will hang
| return true; | ||
| } | ||
|
|
||
| if (session.storeWrapper?.itemBroker is not null) |
There was a problem hiding this comment.
If it is null then you should abort with an error
| } | ||
| } | ||
|
|
||
| while (!RespWriteUtils.WriteInteger(1, ref dcurr, dend)) |
There was a problem hiding this comment.
The client may be unblocked between you checking if it's blocked and attempting to unblock it (in which case you should return 0). In other words, you should get some feedback from the observer that indicates if you initiated the unblocking or not.
| @@ -118,13 +123,15 @@ private async Task<CollectionItemResult> GetCollectionItemAsync(CollectionItemOb | |||
| ? TimeSpan.FromMilliseconds(-1) | |||
| : TimeSpan.FromSeconds(timeoutInSeconds); | |||
|
|
|||
| var isError = false; | |||
There was a problem hiding this comment.
Naming: isError is confusing here, since there's no error actually occurring, should be something like isForceUnblocked.
| try | ||
| { | ||
| // Wait for either the result found notification or the timeout to expire | ||
| await observer.ResultFoundSemaphore.WaitAsync(timeout, observer.CancellationTokenSource.Token); | ||
| } | ||
| catch (OperationCanceledException) | ||
| catch (OperationCanceledException) when (observer.CancellationTokenSource.IsCancellationRequested) |
There was a problem hiding this comment.
This will also be true if the session was disposed
| /// <summary> | ||
| /// Indicates whether the result represents an error. | ||
| /// </summary> | ||
| internal readonly bool IsError { get; } |
There was a problem hiding this comment.
Same issue with naming here, should be something like IsForceUnblocked (update the comment accordingly)
| [Test] | ||
| [TestCase("ERROR", Description = "Multiple unblock attempts with ERROR")] | ||
| [TestCase("TIMEOUT", Description = "Multiple unblock attempts with TIMEOUT")] | ||
| public async Task ClientUnblockMultipleAttemptsTest(string mode) |
There was a problem hiding this comment.
What's the difference between this test and ClientUnblockBasicTest?
| [Test] | ||
| [TestCase("ERROR", Description = "Multiple unblock attempts with ERROR")] | ||
| [TestCase("TIMEOUT", Description = "Multiple unblock attempts with TIMEOUT")] | ||
| public async Task ClientUnblockMultipleAttemptsTest(string mode) |
There was a problem hiding this comment.
Can add a couple more tests here, for example -
1 client blocks, send 1 force unblock and 1 add in parallel - check that either client gets the data or gets unblocked and data stays in collection, can repeat test multiple times.
Multiple clients blocking, multiple calls for force unblock & add in parallel - check that no added data gets lost (i.e. either stays in the collection or being returned to clients).
Adding the CLIENT UNBLOCK command to garnet