Skip to content
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

Merged
merged 7 commits into from
Aug 21, 2024

Conversation

bengsparks
Copy link
Contributor

Closes #1299

This PR instructs serde to rename sample_frequency into sample_freq during de/serialization.
It also introduces a module for converting the u8 into a String 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 for nats, 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.

Also adds crate-private module for handling schema differences
@caspervonb
Copy link
Collaborator

caspervonb commented Aug 14, 2024

Looks good to me, as a test one could confirm that the config is reported back as expected.
Maybe split the nats change into its own PR for changelog generation.

@bengsparks
Copy link
Contributor Author

@caspervonb
The PR now includes a test in async-nats/tests/jetstream_tests.rs that can be executed via cargo test jetstream::consumer_configs_sample_frequency, which checks that the cached info of push and pull consumers now contain the correctly set field.
If this PR is given the go-ahead, then I'll revert the commit for the nats module and open a second PR.

Copy link
Member

@Jarema Jarema left a 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.

fnichol and others added 4 commits August 19, 2024 12:55
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 <jkeiser@systeminit.com>
Signed-off-by: Fletcher Nichol <fletcher@systeminit.com>
Signed-off-by: Fletcher Nichol <fnichol@nichol.ca>
Also adds private module from handling schema differences
@bengsparks
Copy link
Contributor Author

bengsparks commented Aug 19, 2024

@Jarema I've reverted the commit that uses the nats module.
I had to fumble around with the commit history a bit (might've accidentally included a commit from another contributor?), but this should be fine to merge (when squashed).

@bengsparks bengsparks requested a review from Jarema August 19, 2024 11:18
Copy link
Member

@Jarema Jarema left a 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!

@Jarema Jarema merged commit f044e06 into nats-io:main Aug 21, 2024
12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

JetStream: SampleFrequency is not set on Pull Consumers
4 participants