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
Add ResumeAtRevision support to KV Watcher #491
Open
niklasfp
wants to merge
6
commits into
nats-io:main
Choose a base branch
from
niklasfp:478-watch-resume-at-revision
base: main
Could not load branches
Branch not found: {{ refName }}
Could not load tags
Nothing to show
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from 5 commits
Commits
Show all changes
6 commits
Select commit
Hold shift + click to select a range
0b03c7d
Add ResumeAtRevision support to KV Watcher
niklasfp a6e1269
Fix doc comments
niklasfp fbc94f0
Remove empty line in test
niklasfp c545c9c
Make ResumeAtRevision init only
niklasfp 38c2295
Simplify if statement
niklasfp 8952bc8
merge main
niklasfp File filter
Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On second thought it might be a little abrasive to throw here, since
IncludeHistory
will default tofalse
but when settingResumeAtRevision
it must be true. I think it makes sense to ignoreIncludeHistory
whenResumeAtRevision
is set.UpdatesOnly = true
andResumeAtRevision > 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, so if I update the docs, on
NatsKVOpts
to mention that it ignoresIncludeHistory
ifResumeAtRevision
is > 0, and thatUpdatesOnly
andResumeAtRevision
is mutually exclusive.At the same time update the docs on
INatsKVStore.WatchAsync
methods to mention that an exception will be thrown ifUpdatesOnly = true
andResumeAtRevision >0
. Then the question is, ArgumentException, InvalidOperationException or something completely different ? 😄It feels dirty to let this go to the server, if we already know it will fail 😜
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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