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

feat: kv.watch support #26

Merged
merged 7 commits into from
Dec 4, 2023
Merged

feat: kv.watch support #26

merged 7 commits into from
Dec 4, 2023

Conversation

lucacasonato
Copy link
Member

This commit adds support for the Database::watch method in both SQLite and remote backends, and the /watch method in the KV Connect protocol.

let codec = LengthDelimitedCodec::builder()
.little_endian()
.length_field_length(4)
.max_frame_length(16 * 1048576)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

extract into const

yield outputs;

let futures = subscriptions.iter_mut().map(|subscription| {
Box::pin(subscription.wait_until_updated(current_versionstamp))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the Box::pin for getting around borrow checker issues? Can it be moved to the outer scope?

// The response to a watch request for a single key.
message WatchKeyOutput {
// Whether the value changed since the last watch delivery.
bool changed = 1;
Copy link
Member

@losfair losfair Nov 13, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How strict should changed be? Does it mean "something might have changed" or "something definitely changed"? Does it mean there must be visible versionstamp change in entry_if_changed, or just "something might have happened between"?

If a key changes from null -> something -> null but watch() only captured the two null states, should it deliver Unchanged or Changed with both entry_if_changed == null?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed. The CLI with raw: false (default) will de-duplicate on versionstamp (so will not redeliver) for this null -> value -> null case.

Copy link
Member

@losfair losfair left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@johnspurlock-skymethod
Copy link
Contributor

Does the /watch endpoint return content-type: application/octet-stream in the http response? (per the spec)

I'm only seeing:

date: Wed, 22 Nov 2023 01:52:32 GMT
transfer-encoding: chunked

@losfair
Copy link
Member

losfair commented Dec 4, 2023

Does the /watch endpoint return content-type: application/octet-stream in the http response? (per the spec)

Fixed

@losfair losfair merged commit 801b32a into main Dec 4, 2023
4 checks passed
@losfair losfair deleted the watch branch December 4, 2023 07:42
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.

None yet

3 participants