feat: Add multi get #53
Conversation
Momento/ScsDataClient.cs
Outdated
| DateTime deadline = DateTime.UtcNow.AddSeconds(this.dataClientOperationTimeoutSeconds); | ||
| try | ||
| { | ||
| _GetResponse resp = await this.grpcManager.Client().GetAsync(request, new Metadata { { "cache", cacheName } }, deadline: deadline); |
There was a problem hiding this comment.
With how we are currently looping through each key and then awaiting an async call here this will end up being a pretty slow call since its doing them 1 at a time. I think we will want to use async tasks and the WhenAll method for allowing us to fire off all the requests in parallel before awaiting their responses.
…harp into feat/addMultiGet
…of seconds for more control
There was a problem hiding this comment.
Nice this is looking good to me now! Maybe have @wandaitzuchen take pass as well get another pair 👀 on it as well.
|
@poppoerika is test failure because of issue in alpha still? |
|
@eaddingtonwhite - It failed because of the alpha issue, but it looks like the issue has been resolved? |
|
|
||
| namespace MomentoSdk | ||
| { | ||
| internal sealed class ScsDataClient : IDisposable | ||
| { | ||
| private readonly DataGrpcManager grpcManager; | ||
| private readonly uint defaultTtlSeconds; | ||
| private readonly uint dataClientOperationTimeoutSeconds; | ||
| private const uint DEFAULT_DEADLINE_SECONDS = 5; | ||
| private readonly uint dataClientOperationTimeoutMilliseconds; |
There was a problem hiding this comment.
just curious why switching to milli?
There was a problem hiding this comment.
We thought that more control over request timeout might be needed since we're offering a performant cache product. Also matching with the Python SDK as well as making the failure test possible to make sure multi get accept failed responses as argument.
cc @eaddingtonwhite
Momento/ScsDataClient.cs
Outdated
| { | ||
| tasks.Add(SendMultiGetAsync(cacheName, Convert(key))); | ||
| } | ||
|
|
||
| await Task.WhenAll(tasks); | ||
| foreach (Task<CacheMultiGetResponse> t in tasks) | ||
| { | ||
| if (t.Result.SuccessfulResponse() is not null) | ||
| { | ||
| successResponses.Add(t.Result.SuccessfulResponse()); | ||
| } | ||
| if (t.Result.FailedResponse() is not null) | ||
| { | ||
| failedResponses.Add(t.Result.FailedResponse()); | ||
| } | ||
| } | ||
|
|
||
| return new CacheMultiGetResponse(successResponses, failedResponses); |
There was a problem hiding this comment.
nit: feel like this is a bit much code repetition. would be nice to clean this up a bit. just a nit.
poppoerika
dismissed stale reviews from wandaitzuchen and eaddingtonwhite
via
0512667
Add
MultiGetAsycfollowing the example ofmulti_getin the Python SDK.Closes #52