wayfair-incubator / AeroSharp

Aerospike client wrapper for C#
https://wayfair-incubator.github.io/AeroSharp/
Apache License 2.0
5 stars 11 forks source link

Enhance ReadModifyWrite Method to support Timeout.InfiniteTimeSpan #178

Closed awesor closed 10 months ago

awesor commented 1 year ago

Problem Statement

We seem to not really respect a specific TimeSpan option of InfiniteTimeSpan (where the millisecond attribute is set to -1 and everything else is 0). Users of the ReadModifyWrite methods may think that their TimeSpan is being respected as it was set in the WriteConfiguration, but in reality, we're replacing the TTL TimeSpan value with the TimeSpan.FromSeconds and the Write Behavior to be "SetOnWrite."

Proposed Solution

I think that we should make this specific policy of changing the TTL a bit clearer in both the docs and the comments of the method. That way, when users go to add the method, they can be alerted to the fact that the TTL is using seconds. Additionally, we should try to support the specific InfiniteTimeSpan type and convert that to a -1 integer for the expiration.

Alternatives Considered

Another thought could be that we just keep things as is, but provide more descriptions, but I do think that enhancing the method where we convert the TotalSeconds is our best bet.

We could also just modify the method: WriteAsyncWithEqualGeneration

https://github.com/wayfair-incubator/AeroSharp/blob/ce2f061bcf643924f51d891fb60c6e9a75a11a6e/src/AeroSharp/DataAccess/KeyValueAccess/KeyValueStore.cs#L149C22-L149C51

That way, we don't have to wait until the mapper, and instead just do the conversion to seconds there.

Additional Context

Specifically, I think the changes should happen here: https://github.com/wayfair-incubator/AeroSharp/blob/ce2f061bcf643924f51d891fb60c6e9a75a11a6e/src/AeroSharp/DataAccess/Policies/WriteConfigurationToWritePolicyMapper.cs#L43

farmerau commented 1 year ago

Because this is in WriteConfigurationToWritePolicyMapper, I think this has little to do with RMW except that RMW defaults to TimeToLiveBehavior.SetOnWrite.

This behavior with Timeout.InfiniteTimeSpan would happen with all users who set TimeToLiveBehavior to TimeToLiveBehavior.SetOnWrite, not just the RMW case.

I think correcting it there, though, should resolve the behavior in all cases.

Fixing it in WriteAsyncWithEqualGeneration would only fix it for RMW, which is probably worse because then the behavior is even further made to be different.

awesor commented 1 year ago

That's a great callout - I didn't think about that as I was focusing too tightly on my use case. Great point, I agree that we should keep this on WriteConfigurationToWritePolicyMapper

github-actions[bot] commented 11 months ago

Automatically marking issue as stale due to lack of activity

github-actions[bot] commented 10 months ago

Automatically closing this issue as stale