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.

Sign up for GitHub

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

Jump to bottom

ENG-3044: Introduce set streaming support #589

Merged
merged 1 commit into from
Feb 1, 2022
Merged

ENG-3044: Introduce set streaming support #589

merged 1 commit into from
Feb 1, 2022

Conversation

erickpintor
Copy link
Contributor

Changes:

  • New event type 'set' added;
  • Added type information for set events;
  • Test cases.

faunaee
faunaee suggested changes Jan 26, 2022
View reviewed changes
Copy link
Contributor

@faunaee faunaee left a comment

Choose a reason for hiding this comment

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

Works pretty well. But a couple of small changes could make it better.

macnealefauna
macnealefauna previously approved these changes Jan 28, 2022
View reviewed changes
@macnealefauna macnealefauna requested a review from faunaee January 28, 2022 21:07
faunaee
faunaee previously approved these changes Jan 28, 2022
View reviewed changes
Copy link
Contributor

@faunaee faunaee left a comment

Choose a reason for hiding this comment

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

LGTM! 👍

@erickpintor erickpintor dismissed stale reviews from faunaee and macnealefauna via 0bfb9f2 January 28, 2022 21:55
@faunaee
Copy link
Contributor

faunaee commented Jan 31, 2022

This PR likely needs to be re-based on main, to bring in the changes for version 4.4.2.

@faunaee
Copy link
Contributor

faunaee commented Jan 31, 2022

Also, the stream tests are failing due to reaching a timeout. It would be great to fix those tests so that they complete successfully.

@erickpintor
Copy link
Contributor Author

This is currently based on top of main. The tests will be failing until the next release of the fauna/faunadb docker image.

faunaee
faunaee approved these changes Jan 31, 2022
View reviewed changes
Copy link
Contributor

@faunaee faunaee left a comment

Choose a reason for hiding this comment

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

LGTM! 👍

@elersong elersong merged commit fd0960e into main Feb 1, 2022
@elersong elersong deleted the set-streaming branch February 1, 2022 19:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@faunaee faunaee faunaee approved these changes

@macnealefauna macnealefauna macnealefauna left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@erickpintor @faunaee @macnealefauna @elersong