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.

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

chore: update list of retryable gRPC functions #595

Merged
merged 1 commit into from
Dec 6, 2024
Merged
Changes from all commits
Commits
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
81 changes: 56 additions & 25 deletions src/Momento.Sdk/Internal/Retry/DefaultRetryEligibilityStrategy.cs
Original file line number Diff line number Diff line change
@@ -1,10 +1,11 @@
#pragma warning disable 1591
using System;
using System.Collections.Generic;
using System.Linq;
using Grpc.Core;
using Microsoft.Extensions.Logging;
using Microsoft.Extensions.Logging.Abstractions;
using Momento.Protos.CacheClient;
using Momento.Protos.CacheClient.Pubsub;
using Momento.Sdk.Config.Retry;

namespace Momento.Sdk.Internal.Retry
Expand All @@ -16,7 +17,7 @@ namespace Momento.Sdk.Internal.Retry
/// </summary>
public class DefaultRetryEligibilityStrategy : IRetryEligibilityStrategy
{
private readonly HashSet<StatusCode> _retryableStatusCodes = new HashSet<StatusCode>
private readonly HashSet<StatusCode> _retryableStatusCodes = new()
{
//StatusCode.OK,
//StatusCode.Cancelled,
Expand All @@ -37,35 +38,59 @@ public class DefaultRetryEligibilityStrategy : IRetryEligibilityStrategy
//StatusCode.DataLoss,
};

private readonly HashSet<Type> _retryableRequestTypes = new HashSet<Type>
private readonly HashSet<Type> _retryableRequestTypes = new()
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Compare to the Go SDK version.

{
typeof(_SetRequest),
typeof(_GetRequest),
typeof(_GetBatchRequest),
typeof(_SetRequest),
typeof(_SetBatchRequest),
// Not retryable: typeof(_SetIfRequest),
// SetIfNotExists is deprecated
// Not retryable: typeof(_SetIfNotExistsRequest),
typeof(_DeleteRequest),
typeof(_DictionarySetRequest),
// not idempotent: typeof(_DictionaryIncrementRequest),
typeof(_KeysExistRequest),
// Not retryable: typeof(_IncrementRequest),
// Not retryable: typeof(_UpdateTtlRequest),
typeof(_ItemGetTtlRequest),
typeof(_ItemGetTypeRequest),

typeof(_DictionaryGetRequest),
typeof(_DictionaryFetchRequest),
typeof(_DictionarySetRequest),
// Not retryable: typeof(_DictionaryIncrementRequest),
typeof(_DictionaryDeleteRequest),
typeof(_DictionaryLengthRequest),

typeof(_SetFetchRequest),
typeof(_SetSampleRequest),
typeof(_SetUnionRequest),
typeof(_SetDifferenceRequest),
typeof(_SetFetchRequest),
// not idempotent: typeof(_ListPushFrontRequest),
// not idempotent: typeof(_ListPushBackRequest),
// not idempotent: typeof(_ListPopFrontRequest),
// not idempotent: typeof(_ListPopBackRequest),
typeof(_ListFetchRequest),
/*
* Warning: in the future, this may not be idempotent
* Currently it supports removing all occurrences of a value.
* In the future, we may also add "the first/last N occurrences of a value".
* In the latter case it is not idempotent.
*/
typeof(_SetContainsRequest),
typeof(_SetLengthRequest),
// Not retryable: typeof(_SetPopRequest),

// Not retryable: typeof(_ListPushFrontRequest),
// Not retryable: typeof(_ListPushBackRequest),
// Not retryable: typeof(_ListPopFrontRequest),
// Not retryable: typeof(_ListPopBackRequest),
// Not used: typeof(_ListEraseRequest),
typeof(_ListRemoveRequest),
typeof(_ListFetchRequest),
typeof(_ListLengthRequest),
// not idempotent: typeof(_ListConcatenateFrontRequest),
// not idempotent: typeof(_ListConcatenateBackRequest)
// Not retryable: typeof(_ListConcatenateFrontRequest),
// Not retryable: typeof(_ListConcatenateBackRequest),
// Not retryable: typeof(_ListRetainRequest),

typeof(_SortedSetPutRequest),
typeof(_SortedSetFetchRequest),
typeof(_SortedSetGetScoreRequest),
typeof(_SortedSetRemoveRequest),
// Not retryable: typeof(_SortedSetIncrementRequest),
typeof(_SortedSetGetRankRequest),
typeof(_SortedSetLengthRequest),
typeof(_SortedSetLengthByScoreRequest),

typeof(_SubscriptionRequest)
};

private readonly ILogger _logger;
Expand Down Expand Up @@ -100,20 +125,26 @@ public bool IsEligibleForRetry<TRequest>(Status status, TRequest request)

public override bool Equals(object obj)
{
if ((obj == null) || !this.GetType().Equals(obj.GetType()))
// ReSharper disable once ConditionIsAlwaysTrueOrFalseAccordingToNullableAPIContract
if (obj is null || obj.GetType() != GetType())
{
return false;
}

var other = (DefaultRetryEligibilityStrategy)obj;
return _retryableRequestTypes.SetEquals(other._retryableRequestTypes) &&
_retryableStatusCodes.SetEquals(other._retryableStatusCodes);
_retryableStatusCodes.SetEquals(other._retryableStatusCodes);
}

public override int GetHashCode()
{
return base.GetHashCode();
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 have several classes that use base.GetHashCode(), which violates the contract that two objects that equal each other should have the same hash code. They are mostly unimportant like this one, but we should update them when we see them.

unchecked
{
var hash = _retryableRequestTypes.Aggregate(17,
(current, type) => current * 31 + (type?.GetHashCode() ?? 0));
return _retryableStatusCodes.Aggregate(hash,
(current, code) => current * 31 + code.GetHashCode());
}
}
}
}

}
Loading