nats-io / nats.net.v1

The official C# Client for NATS
Apache License 2.0
646 stars 151 forks source link

Key Value TryCreate/TryUpdate #538

Closed thinkbeforecoding closed 4 months ago

thinkbeforecoding commented 2 years ago

Feature Request

Key Value TryCreate/TryUpdate

Use Case:

The Key Value features are excellent, enabling simple synchronization mechanisms as Leader election etc. using the Create/Update methods that check expected revision before storing proposed value.

Proposed Change:

The Create and Update methods currently throw exceptions, but it would be useful to add new TryCreate/TryUpdate methods that indicates success/failure without throwing an exception.

Who Benefits From The Change(s)?

Throwing/Catching exceptions is costly, and does not make code simpler when it is expected that the call could fail.

Alternative Approaches

Catching the exception to return a boolean, but it makes code less efficient and hard to read.

thinkbeforecoding commented 2 years ago

For now, the exception is thrown in PublishAck.Init(): https://github.com/nats-io/nats.net/blob/1e5f0fb6b63a9b9c578613b95ff32e9899fb19e8/src/NATS.Client/JetStream/PublishAck.cs#L35

Avoiding the exception thus require to throw this exception only in some cases.

thinkbeforecoding commented 2 years ago

To implement a TryCreate/TryUpdate correctly, we need to implement a TryPublish that is returning the PublishAck without raising an exception in case of an error code. This could be kept private if we don't want to change the Api, but it could be a good idea to provide it for other scenarios.

thinkbeforecoding commented 2 years ago

Eventually we'll want to implement also UpdateAsync and TryUpdateAsync.

thinkbeforecoding commented 2 years ago

For the Method/TryMethod pattern I usually proceed like this: Implement the TryMethod first that doesn't raise an exception but indicates a success or error. Implement then Method by calling TryMethod and raising an exception in case of error. This avoids reimplementing everything twice.

thinkbeforecoding commented 2 years ago

Would it be ok to add a flag in PublishOptions to indicate that Publish should not raise exception on error ? This could avoid passing an extra parameter along the chain. The response parsing would use it to know if it raises the exception directly when parsing the Publish response.

scottf commented 4 months ago

Closing as will not be done