feat: dictionary implementation #88

malandis · 2022-07-26T15:58:51Z

This PR:

Creates a separate incubating client
Implements the core dictionary functionality (DictionarySet, DictionaryGet, DictionaryDelete, DictionaryRemoveField and their multi counterparts).

For the dictionary implementation, we created a separate incubating client. In the future the incubating client will be in a separate repository; for now it is in a separate namespace. In preparation for this we made the incubating client only depend on a client interface. This interface defines the contract that a production client should implement. We chose this pattern as opposed to inheritance or reflection to: (1) loosely couple the incubating client to the production one, and (2) to be able to have client documentation available to both the production and incubating clients.

To limit accessibility of internal code, we refactored to a separate namespace. To share with the incubating client, we increased visibility where necessary.

In preparation for the repository split, the incubating client has its own set of integration tests.

This commit refactors the constructor to pass the host to the control- and data-client constructors. These constructors in turn pass the host down to the control- and data-grpc managers, where we build the URL. That way SimpleCacheClient does not have to build the URL.

For the initial implementation, we did the following: - Created a separate integration test project to test the incubating package - Moved non-public classes from MomentoSdk to a separate MomentoSdk.Internal namespace - Made MomentoSdk.Internal classes that need to be re-used in the incubating code - Refactored ScsDataClient into base class and derived so the incubating client can re-use common functionality

Because the dictionary features are still on special partitions, use a separate cache from the other integration tests.

kvcache · 2022-07-27T19:03:24Z

I'm reading this but it's quite big, thanks for your patience.
Please note for merging that you will need to adjust the commit message profoundly, as the squash of all these commit messages does not represent the change very well.

kvcache

Nice start - there are a few things I noticed in here; and probably there are things I missed due to the size. More eyes would be good.

Also looks like the names in here aren't matching the proposal in The Document.

kvcache · 2022-07-27T19:11:58Z

Momento/ISimpleCacheClient.cs

+    /// <returns>Task containing the result of the delete operation.</returns>
+    public CacheDeleteResponse Delete(string cacheName, byte[] key);


this doesn't return a Task. (many of these comments refer to Task response which is not present)

why do we need these non-task methods again?

this doesn't return a Task. (many of these comments refer to Task response which is not present)

Thanks. Must have had a typo.

why do we need these non-task methods again?

I proposed getting rid of them. It's still a proposal.

I see green lines here - can you justify the presence of these non-task methods in the C# sdk?

Will revisit in next PR

Fixed the comments in 1181245

kvcache · 2022-07-27T19:19:41Z

Momento/Incubating/Internal/ScsDataClient.cs

+            var response = this.grpcManager.Client.DictionaryGet(request, MetadataWithCache(cacheName), deadline: CalculateDeadline());
+            return new CacheDictionaryGetResponse(response.DictionaryBody[0]);


this subscript seems.. hopeful.

It's already in a try-except block where an out-of-bounds exception would be caught, then thrown as a ClientSdkException. I left a TODO in b0277bb for our upcoming exception handling UX revamp.

kvcache · 2022-07-27T19:28:31Z

Momento/Incubating/Internal/ScsDataClient.cs

+
+    private async Task<CacheDictionaryGetMultiResponse> SendDictionaryGetMultiAsync(string cacheName, string dictionaryName, IEnumerable<ByteString> fields)
+    {
+        _DictionaryGetRequest request = new() { DictionaryName = Convert(dictionaryName) };


I've been out of c# for a while; implicit new target from context. Nice idea.

It's pretty nifty. The above is equivalent to:

_DictionaryGetRequest request = new _DictionaryGetRequest(); request.DictionaryName = Convert(dictionaryName);

kvcache · 2022-07-27T19:32:13Z

Momento/Incubating/Responses/CacheDictionaryGetMultiResponse.cs

+    public List<string?> Strings()
    {
-        throw new NotImplementedException();
+        return responses.Select(response => response.String()).ToList();
    }


I wonder if instead of doubling the static memory cost of responses we should let users opt in to that.

Why not return IEnumerable<string?> instead and omit ToList()?

(here and everywhere)

Why not return IEnumerable<string?> instead and omit ToList()?

Good call. I think I converted to list when intiially writing tests. Not necessary.

Fixed in 7c84e5a

kvcache · 2022-07-27T19:33:44Z

Momento/Incubating/Responses/CacheDictionaryGetResponse.cs

+    public string? String()
    {
-        throw new NotImplementedException();
+        if (Bytes == null)
+        {
+            return null;
+        }
+        return Encoding.UTF8.GetString(Bytes);
    }


Why isn't this a property?

(1) Want to remind users it's doing something non-trivial (string encoding)
(2) String encoding could raise an exception if the user calls String() on something from the cache that wasn't a string. I think it's bad form to have a property that could raise an exception

So call it DecodeString or AsString?

We'll flesh out a spec for the names so they are consistent across SDKs, then sweep through these in a future PR.

kvcache · 2022-07-27T19:40:27Z

Momento/Incubating/Responses/CacheDictionarySetResponse.cs

    {
-        throw new NotImplementedException();
+        return (string)field;


again with object. This is not how C# does type conversions.

you're going to raise this or something like it whenever your users invoke this method:

[System.InvalidCastException: Unable to cast object of type 'System.Byte[]' to type 'System.String'.]

Don't use object 😄 (here and everywhere)

After discussing what the set response objects should be doing, I removed this code in b3867ee.

kvcache · 2022-07-27T19:41:09Z

Momento/Incubating/Responses/CacheDictionarySetResponse.cs

    }

-    public string Key(Encoding? encoding = null)
+    public string FieldToString()


Why aren't these properties? (here and everywhere)

#88 (comment)

kvcache · 2022-07-27T19:52:30Z

Momento/Incubating/SimpleCacheClient.cs

+        if (cacheName == null)
+        {
+            throw new ArgumentNullException(nameof(cacheName));
+        }
+        if (dictionaryName == null)
+        {
+            throw new ArgumentNullException(nameof(dictionaryName));
+        }
+        if (field == null)
+        {
+            throw new ArgumentNullException(nameof(field));
+        }
+        if (value == null)
+        {
+            throw new ArgumentNullException(nameof(value));
+        }


public static void ArgumentNotNull(object value, string argumentName) { if (value == null) { throw new ArgumentNullException(argumentName); } }

used like

ArgumentNotNull(cacheName, nameof(cacheName));

or can you just use [NotNull]? https://docs.microsoft.com/en-us/dotnet/api/system.diagnostics.codeanalysis.notnullattribute?view=net-6.0

either of these strategies will reduce unnecessary boilerplate line noise.

Already considered ArgumentNotNull but it's not available in the version we are using 😢 .

I was not aware of the NotNull attribute and will check it out. Thanks.

Fixed in ea02843

looks much nicer, and removes over 400 lines 🎉

kvcache · 2022-07-27T19:56:52Z

Momento/Internal/ScsDataClient.cs

+    protected ByteString Convert(byte[] bytes)
+    {
+        return ByteString.CopyFrom(bytes);
+    }
+
+    protected ByteString Convert(string s)
+    {
+        return ByteString.CopyFromUtf8(s);
+    }


maybe you want to call these AsByteString?

Gone a step further and indulged myself in C# features. In 33d84b0 I implemented these as extension methods to byte[] and string.

kvcache · 2022-07-27T19:59:53Z

Momento/Internal/DataGrpcManager.cs

 {
    private readonly GrpcChannel channel;
    public Scs.ScsClient Client { get; }

    private readonly string version = "csharp:" + GetAssembly(typeof(MomentoSdk.Responses.CacheGetResponse)).GetName().Version.ToString();

-    internal DataGrpcManager(string authToken, string endpoint)


endpoint is the correct word for this.

Even if it's just the domain name?

Reverted in 06d2cca

Refact the `Convert` method to be an extension method to byte array and string types. With this change the developer types `ToByteString` to convert the underlying type.

kvcache

cool, I'm on board.

cprice404 · 2022-07-28T19:08:33Z

I haven't had time to start looking at the code yet - hope to do that right after lunch. But I am 💯 on the description of the choices about how to structure the incubating client, interface, accessors, etc.

malandis · 2022-07-28T19:09:53Z

I haven't had time to start looking at the code yet - hope to do that right after lunch. But I am 100 on the description of the choices about how to structure the incubating client, interface, accessors, etc.

Thank you. I was seeking your feedback about this in particular.

In C# byte array comparisons are by reference. This will likely cause unexpected behavior to end-users. To make things do what we mean, we provide a structural comprarer that works on the contents as opposed to the array reference. We also provide a `DictionaryEquals` extension that parallels the C# library's `SetEquals` method. We have to do this because, even if we provide a structural comparer as described above, this only operates on the keys, not the values.

cprice404

Reviewed in more detail. I like what you've done with the interface. 🚢 from me as soon as the more relevant stakeholders are happy :)

cprice404 · 2022-07-28T20:13:19Z

Momento/ISimpleCacheClient.cs

+using MomentoSdk.Responses;
+namespace MomentoSdk;
+
+public interface ISimpleCacheClient : IDisposable


cprice404 · 2022-07-28T20:17:57Z

Momento/Incubating/SimpleCacheClient.cs

+    private readonly ISimpleCacheClient simpleCacheClient;
+    private readonly ScsDataClient dataClient;
+
+    public SimpleCacheClient(ISimpleCacheClient simpleCacheClient, string authToken, uint defaultTtlSeconds, uint? dataClientOperationTimeoutMilliseconds = null)


not a blocker for this PR, but I would probably expect this constructor to be private, and we would have a public one (or factory method) that handles the construction of the other client implicitly so the user doesn't have to.

Or perhaps you already have a builder somewhere that I haven't seen yet that takes care of this concern.

cprice404 · 2022-07-28T20:18:54Z

Momento/Incubating/SimpleCacheClientFactory.cs

+
+public class SimpleCacheClientFactory
+{
+    public static SimpleCacheClient Get(string authToken, uint defaultTtlSeconds, uint? dataClientOperationTimeoutMilliseconds = null)


Ah, here's the builder :) ignore previous comment.

Is this a typical naming convention (*Factory.Get) for builders in CS?

Is this a typical naming convention (*Factory.Get) for builders in CS?

I poked around and didn't see a consensus. I was learning toward CreateClient as opposed to Get. Thoughts?

`CreateClient` is more approachable.

malandis added 10 commits July 25, 2022 16:53

Bump protos to latest version.

73d3fc0

Refactor incubating client: loosely couple to production client.

df33bf4

Implement DictionarySet and DictionaryGet with bytes.

f7c8a7f

Implement DictionaryGetAsync and DictionarySetAsync.

201e65f

Broaden DictionarySetMulti key-value pair type.

2621a83

Add null reference checks.

5779498

Prefer Unix newline separators.

e2f8c19

Update timings for refresh ttl tests.

961e697

malandis marked this pull request as draft July 26, 2022 15:59

malandis added 14 commits July 26, 2022 09:24

DRY data client metadata creation.

ca31434

Use a separate cache for the incubating tests.

95d7368

Because the dictionary features are still on special partitions, use a separate cache from the other integration tests.

Implement DictionaryGetAll.

887ee17

Implement DictionarySetMulti and DictionarySetMultiAsync.

e2c4ba3

Implement DictionaryGetMulti and DictionaryGetMultiAsync.

aff4f34

DRY data client code.

72aee8f

Put Utils class in MomentoSdk.Internal and share with Incubating.

36ac273

Backfill null argument reference checks.

784971c

Use this consistently in tests.

e319831

Implement DictionaryDelete and DictionaryDeleteAsync.

6f020c9

Implement DictionaryRemoveField and DictionaryRemoveFieldAsync.

b0c8c85

Implement DictionaryRemoveFields and DictionaryRemoveFieldsAsync.

abb6c5c

Correct JwtUtils namespace.

187caa8

Document dictionary methods.

e699c4b

malandis marked this pull request as ready for review July 27, 2022 15:34

Mildy improve readability.

25ec412

malandis requested a review from kvcache July 27, 2022 19:09

kvcache reviewed Jul 27, 2022

View reviewed changes

malandis added 9 commits July 27, 2022 13:18

Do not wrap responses in lists.

7c84e5a

Add newline to end of files.

0c3a8c0

Refactor argument null checks to utility method.

ea02843

Check for null elements of containers.

f60b47f

Refactor Convert to extension methods.

33d84b0

Refact the `Convert` method to be an extension method to byte array and string types. With this change the developer types `ToByteString` to convert the underlying type.

Remove pass-through data from dictionary set response objects.

b3867ee

Use "Momento endpoint" terminology.

06d2cca

Leave comment for upcoming error handling UX revamp.

b0277bb

Fix comments referencing tasks.

1181245

malandis requested review from kvcache and cprice404 July 28, 2022 16:13

kvcache previously approved these changes Jul 28, 2022

View reviewed changes

malandis dismissed kvcache’s stale review via c5734b0 July 28, 2022 19:31

Fix ugly whitespace.

acd4b88

cprice404 previously approved these changes Jul 28, 2022

View reviewed changes

Document the rest of the dictionary methods.

934ebce

malandis dismissed cprice404’s stale review via 934ebce July 28, 2022 20:41

malandis added 2 commits July 28, 2022 16:09

Prefer name CreateClient in Factory.

b960b9b

`CreateClient` is more approachable.

Update incubating README

672871b

malandis requested a review from kvcache July 28, 2022 23:12

kvcache approved these changes Jul 29, 2022

View reviewed changes

malandis linked an issue Jul 29, 2022 that may be closed by this pull request

dictionary API SDK implementation #89

Closed

malandis merged commit c040694 into main Jul 29, 2022

malandis deleted the feat/dictionary-impl branch July 29, 2022 14:44

		/// <returns>Task containing the result of the delete operation.</returns>
		public CacheDeleteResponse Delete(string cacheName, byte[] key);

		var response = this.grpcManager.Client.DictionaryGet(request, MetadataWithCache(cacheName), deadline: CalculateDeadline());
		return new CacheDictionaryGetResponse(response.DictionaryBody[0]);

feat: dictionary implementation #88

feat: dictionary implementation #88

Conversation

malandis commented Jul 26, 2022 • edited by anp13 Loading

kvcache commented Jul 27, 2022

kvcache left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

malandis Jul 28, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

malandis Jul 27, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kvcache left a comment

Choose a reason for hiding this comment

cprice404 commented Jul 28, 2022

malandis commented Jul 28, 2022

cprice404 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

malandis commented Jul 26, 2022 •

edited by anp13

Loading

malandis Jul 28, 2022 •

edited

Loading

malandis Jul 27, 2022 •

edited

Loading