-
Notifications
You must be signed in to change notification settings - Fork 7
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 initial event feed support #290
Conversation
2a4a330
to
c5ace89
Compare
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.
Narrow +1 on the README aside from some nits. Thanks!
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 is looking great! I don't have any really major feedback. Closest question to major is how to handle start_ts
and cursor
input.
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.
Let's goooo
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.
marking as request changes to prevent merge - the only feedback i think is worth blocking over is retryable delaying request 0.
that's not an issue, removing the request changes flag.
02eb2df
Co-authored-by: echo-bravo-yahoo <ashton.eby@gmail.com>
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.
Just some comments on timeouts.
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.
Thanks! Looks good overall. Few bigger pieces of feedback:
-
We're using "event source" in place of "stream token" now. I left comments for all the refs in the README, but we prolly also want to update this in the implementation.
-
Bryan requested that we give Event Feeds top billing so I'd move that section before the Event Streaming one.
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.
Narrow +1 on the docs. I left some nit suggestions, but they're not blocking IMO.
This PR adds support for change feeds in the JS driver. To do this, we add a new
FeedClient
that can be initialized viaClient.feed(eventSource)
.FeedClient
can then be used as a async generator for each page or event by event viaflatten
.nextPage
is exposed for consumers who don't want to use the generator pattern.Or via flatten:
Description
Generally, this PR has surfaced the underlying abstractions aren't really equipped to support more Fauna endpoints. There's a refactoring opportunity across clients and the httpclient, but those changes are likely backwards incompatible. So I tried my best to follow the existing conventions in the codebase.
FeedClient
: Exposes change feed pagination via an async iterator via a publicnextPage
method. It also surfacesflatten
to iterate through all events directly.FeedClientConfiguration
: The required configuration for the iterator. Primarily, optionalstart_ts
andcursor
.FeedPage
: Because there are errors that are embedded inevents
that are apparently fatal for the page,FeedPage
wraps the events wire response in an iterator. If it sees an event type of "error", it throws. I'm not sure that's idiomatic behavior in JS (since you can continue to fetch pages after this event), but I kept parity with the python driver. Additionally,FeedPage
provides public attributes to access meta data about the current page, such as the current cursor.HTTPClient
generics:HTTPClient
interfaces have been updated to support generics. This is the first new API usingrequest
other than query.wire-protocol.ts
: Updated with the wire protocol as I understand it.package.json
: Updated to use the change feed env flag in docker calls. This will need to be removed when the image is updated.Updates to naming:
The following changes have been added to address the renaming of this feature:
EventSource
: An interface to represent the output ofeventSource()
andeventsOn()
.EventSource
replaces all publicly exported uses ofStreamToken
.StreamToken
can be used anywhereEventSource
is specified. This seemed the best way to move towards the correct language while keeping this feature backwards compatible.eventSource()
andeventsOn()
: README and docstring uses oftoStream()
andchangesOn()
have been replaced with these new methods. Feed tests use these new FQL methods. Existing streaming tests have not been changes at all to prove backwards compatibility.Motivation and context
To support change feeds beta in the JS driver.
How was the change tested?
Unit, function, and integration tests were added for all new surface areas.
Change types
Checklist:
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.