-
Notifications
You must be signed in to change notification settings - Fork 172
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
De/Serialize sample_frequency
correctly for Push and Pull Consumers
#1300
Conversation
Also adds crate-private module for handling schema differences
Looks good to me, as a test one could confirm that the config is reported back as expected. |
@caspervonb |
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.
This looks good @bengsparks !
Please remove the sync nats from this PR and it is good to merge.
This change fixes an issue when using a JetStream K/V store where a user is creating, deleting, and re-creating keys. If the last entry for a key is a `Operation::Delete` or `Operation::Purge`, the initial `self.update()` returns an error, causing the second part of the method to be exercised. Prior to this change, if the entry was deleted or purged a `kv.put()` call is used which ignores the revision of that last entry. A single writer to the K/V store would succeed (as no other writers would write first) so no problem. However, if 2 writers attempt to create a key, then a second writer *could* call the `kv.put()` before the first writer calls `kv.put()`. This means that *both* writers get an `Ok(revision)` and can assume that they won the creation of the key. When using a "distributed lock" pattern (that is many writers race to create a key and the first successful writer wins), this above scenario results in potentially more than one writer who believes they have uniquely acquired the distributed lock. This change replaces the `kv.put()` call to a `kv.update()` call and provides the `revision` from the deleted/purged entry to ensure that no other writer has beaten the caller to this update. This change closes the race period between concurrent writers to between the first update and the second update call with some optimistic write concurrency to detect another writer. It appears as though this strategy is in effect in the Go client code [kv.Create] implementation. [kv.Create]: https://github.com/nats-io/nats.go/blob/278f9f188bca4d7bdee283a0e98ab66b82530c60/jetstream/kv.go#L944-L963 Co-authored-by: John Keiser <[email protected]> Signed-off-by: Fletcher Nichol <[email protected]> Signed-off-by: Fletcher Nichol <[email protected]>
Also adds private module from handling schema differences
This reverts commit f2b24ca.
This reverts commit 8493766.
@Jarema I've reverted the commit that uses the |
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.
LGTM!
Thank you for your contribution!
Closes #1299
This PR instructs
serde
to renamesample_frequency
intosample_freq
during de/serialization.It also introduces a module for converting the
u8
into aString
due to nats-server's handling of the SampleFrequency field.The PR is split into two separate commits, one for
async-nats
and the other fornats
, so that the latter can be reverted if it is not wanted.This PR does not contain any tests, but used the setup from #1299 to check if the field was being correctly set.