Add ResumeAtRevision support to KV Watcher #491
Conversation
|
@mtmk I'm seeing some test flappers in Watcher_reconnect_with_history and Watcher_timeout_reconnect locally (also without my changes) Machine: Mac M2 Pro arm64 |
I'm not surprised. they're most stable on Linux at the moment. We also have #464 to deal with Windows flappers. we might open one for Mac too. I'm guessing it's mostly around managing the server process and sockets, but I might be wrong. |
| /// <remarks> | ||
| /// Setting this to a non-zero value will cause the watcher to ignore the values for <see cref="IncludeHistory"/> and <see cref="UpdatesOnly"/>. | ||
| /// </remarks> |
There was a problem hiding this comment.
Is it more consistent/correct to annotate that this option silently overrides other options? Or should we fail with an Exception that states that two options that are mutually exclusive have been set?
There was a problem hiding this comment.
Probably whichever pattern we follow in Consumer Creation is the same pattern we would want to follow here
There was a problem hiding this comment.
@caleblloyd I initially considered throwing 😄 I'll take a look what happens in other places. (any pointers to where we do stuff like this ?)
There was a problem hiding this comment.
On second thought it might be a little abrasive to throw here, since IncludeHistory will default to false but when setting ResumeAtRevision it must be true. I think it makes sense to ignore IncludeHistory when ResumeAtRevision is set.
UpdatesOnly = true and ResumeAtRevision > 0 are 2 non-defaults though so I think it makes sense to throw when those are both set.
Regular Consumer Crerate isn't checking any pre-conditions it looks like, it's taking the Deliver Policy and Start Seq / Start Time separately. So if the Deliver Policy doesn't match the Server will fail with an error, and an exception will be thrown.
There was a problem hiding this comment.
On second thought it might be a little abrasive to throw here, since
IncludeHistorywill default tofalsebut when settingResumeAtRevisionit must be true. I think it makes sense to ignoreIncludeHistorywhenResumeAtRevisionis set.
UpdatesOnly = trueandResumeAtRevision > 0are 2 non-defaults though so I think it makes sense to throw when those are both set.
Ok, so if I update the docs, on NatsKVOpts to mention that it ignores IncludeHistory if ResumeAtRevision is > 0, and that UpdatesOnly and ResumeAtRevision is mutually exclusive.
At the same time update the docs on INatsKVStore.WatchAsync methods to mention that an exception will be thrown if UpdatesOnly = true and ResumeAtRevision >0. Then the question is, ArgumentException, InvalidOperationException or something completely different ? 😄
Regular Consumer Crerate isn't checking any pre-conditions it looks like, it's taking the Deliver Policy and Start Seq / Start Time separately. So if the Deliver Policy doesn't match the Server will fail with an error, and an exception will be thrown.
It feels dirty to let this go to the server, if we already know it will fail 😜
There was a problem hiding this comment.
Calling GetKeys with the resume parameter might be a valid use case. I agree that including history is overridden due to the method overload. However, considering the following points, I suggest keeping it in the options:
-
Cross-cutting functionality: resume seems to be applicable to multiple scenarios and not specific to a single use case. Including it in the options would make it more reusable and accessible across different parts of the codebase.
-
Consistency with other NATS clients: It's important to maintain a certain degree of consistency with other client implementations, unless there are .NET specific differences.
-
Impact on other clients: Altering the method signature or introducing overloads would require updating other client implementations as well. This can be a significant effort, and there should be a strong justification for making such changes unless we can say this is a .NET specific difference.
edit: in terms of validation, since NatsKVWatcher is the last destination for the options, should we validate conflicting options there (e.g. history and ResumeAtRevision)?
There was a problem hiding this comment.
@mtmk I agree with almost everything you wrote there, I just wanted to highlight, that the nats kv watch opts opens up for some "special scenarios"
As for consistency with other clients, I looked at both.net1 and java, IIRC they both introduced overloads (not saying that we should though 😀)
Adding overloads (if done right does not break existing implememtations) (still not saying we should add overloads 😂)
I'll push the validation back or the watcher, and update the docs on all methods?
Since most methods accepting Nats kvopts end up throwing if resume at and include history is set
There was a problem hiding this comment.
I was checking Go and JS implementations https://github.com/nats-io/nats.go/pull/1489/files https://github.com/nats-io/nats.deno/pull/651/files .. so we're not that consistent after all😄
I guess we can add overloads if you think it's going to be a better developer experience. it just feels like a less generic solution to me. wdyt @niklasfp @caleblloyd
There was a problem hiding this comment.
I was checking Go and JS implementations https://github.com/nats-io/nats.go/pull/1489/files https://github.com/nats-io/nats.deno/pull/651/files .. so we're not that consistent after all😄
I guess we can add overloads if you think it's going to be a better developer experience. it just feels like a less generic solution to me. wdyt @niklasfp @caleblloyd
Tbh, overloading it will not be as generic as you stated, and it might be ok to throw at a lover level, as long as we make it clear that some combinations are considered illegal and will throw.
I guess that all methods accepting natkv watch opts should have <exception... docs explaining this?
There was a problem hiding this comment.
I am also in favor of leaving it in Options and adding Validation.
If the Validation needs to run from multiple callers, we could add a internal void ApplyOptsToConsumerConfig(ConsumerConfig config) method on the Options, which performs validation, then mutates a ConsumerConfig?
We do something similar in NatsTlsOpts - where validation is performed at the time of use
|
@mtmk I have not forgotten about this one, I'll get to it this weekend |
Resolves #478