Skip to content
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.

Sign up for GitHub

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

Jump to bottom

feat: data error response update #176

Merged
merged 8 commits into from
Sep 26, 2022
Merged

feat: data error response update #176

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
9d48b99
feat: data response object polymorphism
pgautier404 Sep 23, 2022
a8b5f5f
remove commented out code
pgautier404 Sep 23, 2022
7c3aa6c
fix computation of value in CacheGetResponse
pgautier404 Sep 23, 2022
462abf7
fix a response I missed
pgautier404 Sep 23, 2022
efab849
make response classes abstract
pgautier404 Sep 23, 2022
0aa3060
replace success with hit/miss for get responses
pgautier404 Sep 23, 2022
bee2e90
testing fixes
pgautier404 Sep 23, 2022
5b620a0
make value not nullable
pgautier404 Sep 26, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
53 changes: 39 additions & 14 deletions src/Momento.Sdk/Internal/ScsDataClient.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -115,21 +115,34 @@ public async Task<CacheGetBatchResponse> GetBatchAsync(string cacheName, IEnumer
}
catch (Exception e)
{
throw CacheExceptionMapper.Convert(e);
return new CacheGetBatchResponse.Error(CacheExceptionMapper.Convert(e));
}

// Handle failures
if (continuation.Status == TaskStatus.Faulted)
{
throw CacheExceptionMapper.Convert(continuation.Exception);
return new CacheGetBatchResponse.Error(
CacheExceptionMapper.Convert(continuation.Exception)
);
}
else if (continuation.Status != TaskStatus.RanToCompletion)
{
throw CacheExceptionMapper.Convert(new Exception(String.Format("Failure issuing multi-get: {0}", continuation.Status)));
return new CacheGetBatchResponse.Error(
CacheExceptionMapper.Convert(
new Exception(String.Format("Failure issuing multi-get: {0}", continuation.Status))
)
);
}

// preserve old behavior of failing on first error
foreach (CacheGetResponse response in continuation.Result) {
if (response is CacheGetResponse.Error errorResponse) {
return new CacheGetBatchResponse.Error(errorResponse.Exception);
}
}

// Package results
return new CacheGetBatchResponse(continuation.Result);
return new CacheGetBatchResponse.Success(continuation.Result);
}

public async Task<CacheSetBatchResponse> SetBatchAsync(string cacheName, IEnumerable<KeyValuePair<string, string>> items, uint? ttlSeconds = null)
Expand Down Expand Up @@ -159,19 +172,27 @@ public async Task<CacheSetBatchResponse> SendSetBatchAsync(string cacheName, IEn
}
catch (Exception e)
{
throw CacheExceptionMapper.Convert(e);
return new CacheSetBatchResponse.Error(
CacheExceptionMapper.Convert(e)
);
}

// Handle failures
if (continuation.Status == TaskStatus.Faulted)
{
throw CacheExceptionMapper.Convert(continuation.Exception);
return new CacheSetBatchResponse.Error(
CacheExceptionMapper.Convert(continuation.Exception)
);
}
else if (continuation.Status != TaskStatus.RanToCompletion)
{
throw CacheExceptionMapper.Convert(new Exception(String.Format("Failure issuing multi-set: {0}", continuation.Status)));
return new CacheSetBatchResponse.Error(
CacheExceptionMapper.Convert(
new Exception(String.Format("Failure issuing multi-set: {0}", continuation.Status))
)
);
}
return new CacheSetBatchResponse();
return new CacheSetBatchResponse.Success();
}

private async Task<CacheSetResponse> SendSetAsync(string cacheName, ByteString key, ByteString value, uint? ttlSeconds = null)
Expand All @@ -183,9 +204,9 @@ private async Task<CacheSetResponse> SendSetAsync(string cacheName, ByteString k
}
catch (Exception e)
{
throw CacheExceptionMapper.Convert(e);
return new CacheSetResponse.Error(CacheExceptionMapper.Convert(e));
}
return new CacheSetResponse();
return new CacheSetResponse.Success();
}

private async Task<CacheGetResponse> SendGetAsync(string cacheName, ByteString key)
Expand All @@ -198,9 +219,13 @@ private async Task<CacheGetResponse> SendGetAsync(string cacheName, ByteString k
}
catch (Exception e)
{
throw CacheExceptionMapper.Convert(e);
return new CacheGetResponse.Error(CacheExceptionMapper.Convert(e));
}

if (response.Result == ECacheResult.Miss) {
return new CacheGetResponse.Miss();
}
return new CacheGetResponse(response);
return new CacheGetResponse.Hit(response);
}

private async Task<CacheDeleteResponse> SendDeleteAsync(string cacheName, ByteString key)
Expand All @@ -212,8 +237,8 @@ private async Task<CacheDeleteResponse> SendDeleteAsync(string cacheName, ByteSt
}
catch (Exception e)
{
throw CacheExceptionMapper.Convert(e);
return new CacheDeleteResponse.Error(CacheExceptionMapper.Convert(e));
}
return new CacheDeleteResponse();
return new CacheDeleteResponse.Success();
}
}
21 changes: 20 additions & 1 deletion src/Momento.Sdk/Responses/CacheDeleteResponse.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,27 @@
namespace Momento.Sdk.Responses;

using Momento.Sdk.Exceptions;

public class CacheDeleteResponse
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can't remember if we decided we were going to make these abstract, nbd though

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We did, and I did for the control responses but not the data ones. Fixed in efab849.

{
public CacheDeleteResponse()
public class Success : CacheDeleteResponse { }

public class Error : CacheDeleteResponse
{
private readonly SdkException _error;
public Error(SdkException error)
{
_error = error;
}

public SdkException Exception
{
get => _error;
}

public MomentoErrorCode ErrorCode
{
get => _error.ErrorCode;
}
}
}
87 changes: 71 additions & 16 deletions src/Momento.Sdk/Responses/CacheGetBatchResponse.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,29 +1,84 @@
using System.Collections.Generic;
using System.Linq;
using Momento.Sdk.Exceptions;

namespace Momento.Sdk.Responses;

public class CacheGetBatchResponse
public abstract class CacheGetBatchResponse
{
public List<CacheGetResponse> Responses { get; }

public CacheGetBatchResponse(IEnumerable<CacheGetResponse> responses)
public class Success : CacheGetBatchResponse
{
this.Responses = new(responses);
}
public List<CacheGetResponse> Responses { get; }

public IEnumerable<CacheGetStatus> Status
{
get => Responses.Select(response => response.Status);
}
public Success(IEnumerable<CacheGetResponse> responses)
{
this.Responses = new(responses);
}

public IEnumerable<string?> Strings()
{
return Responses.Select(response => response.String());
public IEnumerable<CacheGetStatus> Status
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@malandis you should look at this part. i'm not super familiar with this part of the API but some random thoughts I have are:

  1. We have been getting rid of the enums in other places, and I had assumed that would happen for CacheGet too, but it seems like it's not happening here.
  2. do we really need 3 separate methods for this? would it not be okay to just have a single enumerable that contains the polymorphic parent type?
  3. in the loops below it looks like we are assuming that all of the responses are Success, but I don't think there is any guarantee that that is actually true?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. You could. I included methods to parallel the unary Get: get the status, get the payload. Since this is a batch operation I also included to get the individual items
  2. That's correct for the way this is implemented now. If this were a single RPC then there would only be on Success

{
get
{
var ret = new List<CacheGetStatus>();
foreach (CacheGetResponse response in Responses) {
if (response is CacheGetResponse.Hit hitResponse) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These look better/safer now, so thanks for changing them.

I'm still feeling iffy about this part of the API as a whole, so I think we should revisit it and have a convo about it later this week, but I think it's okay for now for this PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed, the batch stuff needs some love and we should take a closer look when time allows.

ret.Add(hitResponse.Status);
} else if (response is CacheGetResponse.Miss missResponse) {
ret.Add(missResponse.Status);
}
}
return ret;
}
}

public IEnumerable<string?> Strings()
{
var ret = new List<string?>();
foreach (CacheGetResponse response in Responses)
{
if (response is CacheGetResponse.Hit hitResponse) {
ret.Add(hitResponse.String());
} else if (response is CacheGetResponse.Miss missResponse) {
ret.Add(null);
}
}
return ret.ToArray();
}

public IEnumerable<byte[]?> ByteArrays
{
get
{
var ret = new List<byte[]?>();
foreach (CacheGetResponse response in Responses)
{
if (response is CacheGetResponse.Hit hitResponse)
{
ret.Add(hitResponse.ByteArray);
} else if (response is CacheGetResponse.Miss missResponse) {
ret.Add(null);
}
}
return ret.ToArray();
}
}
}

public IEnumerable<byte[]?> ByteArrays
public class Error : CacheGetBatchResponse
{
get => Responses.Select(response => response.ByteArray);
private readonly SdkException _error;
public Error(SdkException error)
{
_error = error;
}

public SdkException Exception
{
get => _error;
}

public MomentoErrorCode ErrorCode
{
get => _error.ErrorCode;
}
}
}
47 changes: 44 additions & 3 deletions src/Momento.Sdk/Responses/CacheGetResponse.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,11 +1,52 @@
using Google.Protobuf;
using Momento.Protos.CacheClient;

using Momento.Sdk.Exceptions;
namespace Momento.Sdk.Responses;

public class CacheGetResponse : CacheGetResponseBase
public abstract class CacheGetResponse
{
public CacheGetResponse(_GetResponse response) : base(response.Result, response.CacheBody)
public class Hit : CacheGetResponse
{
public CacheGetStatus Status { get => CacheGetStatus.HIT; }
protected readonly ByteString value;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cool, thanks. I think we should merge this as-is because it's a good step forward, but would like to circle back quickly and see if the enum is still necessary at all. It feels like the only thing that is currently forcing us to keep it is that batch api, and if that's true, we should talk about options for getting rid of that bit, because having the enum here feels inconsistent since we got rid of it in the other places.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That sounds good. The enum shouldn't be necessary but removing it altogether as part of this effort seemed risky.


public Hit(_GetResponse response)
{
this.value = response.CacheBody;
}

public byte[] ByteArray
{
get => value.ToByteArray();
}

public string String() => value.ToStringUtf8();
}

public class Miss : CacheGetResponse {
public CacheGetStatus Status {
get => CacheGetStatus.MISS;
}

public Miss() { }
}

public class Error : CacheGetResponse
{
private readonly SdkException _error;
public Error(SdkException error)
{
_error = error;
}

public SdkException Exception
{
get => _error;
}

public MomentoErrorCode ErrorCode
{
get => _error.ErrorCode;
}
}
}
31 changes: 0 additions & 31 deletions src/Momento.Sdk/Responses/CacheGetResponseBase.cs
View file
Open in desktop

This file was deleted.

26 changes: 25 additions & 1 deletion src/Momento.Sdk/Responses/CacheSetBatchResponse.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,29 @@
namespace Momento.Sdk.Responses;

public class CacheSetBatchResponse
using Momento.Sdk.Exceptions;

public abstract class CacheSetBatchResponse
{

public class Success : CacheSetBatchResponse { }

public class Error: CacheSetBatchResponse
{
private readonly SdkException _error;
public Error(SdkException error)
{
_error = error;
}

public SdkException Exception
{
get => _error;
}

public MomentoErrorCode ErrorCode
{
get => _error.ErrorCode;
}
}

}
25 changes: 24 additions & 1 deletion src/Momento.Sdk/Responses/CacheSetResponse.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,28 @@
namespace Momento.Sdk.Responses;

public class CacheSetResponse
using Momento.Sdk.Exceptions;

public abstract class CacheSetResponse
{

public class Success : CacheSetResponse { }

public class Error : CacheSetResponse
{
private readonly SdkException _error;
public Error(SdkException error)
{
_error = error;
}

public SdkException Exception
{
get => _error;
}

public MomentoErrorCode ErrorCode
{
get => _error.ErrorCode;
}
}
}
Loading