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

fix: remove content_sent from StreamingSequences #1351

Merged
merged 2 commits into from
Jul 7, 2023

Conversation

galz10
Copy link
Contributor

@galz10 galz10 commented Jul 7, 2023

Currently, server streaming APIs do not resume from the point of failure. Before removing the contentSent property, the server would keep track of the last content that was sent before failing. To get closure to how server streams currently react to failure, we removed the contentSent property. This means that when we retry the request, the server will send the entire content, rather than starting from where it left off.

@galz10 galz10 requested review from a team as code owners July 7, 2023 18:14
@noahdietz
Copy link
Collaborator

Please run gofmt -w ./server/services to fix the lint failure

Copy link
Collaborator

@noahdietz noahdietz left a comment

Choose a reason for hiding this comment

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

Can you include a little bit more detail the PR description? It's a tad vague how this get's closer to something (which is undefined in the description)

server/services/sequence_service.go Show resolved Hide resolved
@galz10
Copy link
Contributor Author

galz10 commented Jul 7, 2023

Can you include a little bit more detail the PR description? It's a tad vague how this get's closer to something (which is undefined in the description)

updated description

@galz10 galz10 requested a review from noahdietz July 7, 2023 20:03
@noahdietz noahdietz changed the title fix: removed contentSent from StreamingSequences fix: remove contentSent from StreamingSequences Jul 7, 2023
@noahdietz noahdietz changed the title fix: remove contentSent from StreamingSequences fix: remove content_sent from StreamingSequences Jul 7, 2023
@noahdietz noahdietz added the automerge Summon MOG for automerging label Jul 7, 2023
@gcf-merge-on-green gcf-merge-on-green bot merged commit ae910cc into main Jul 7, 2023
9 checks passed
@gcf-merge-on-green gcf-merge-on-green bot deleted the sequence-streaming-fix branch July 7, 2023 20:28
@gcf-merge-on-green gcf-merge-on-green bot removed the automerge Summon MOG for automerging label Jul 7, 2023
gcf-merge-on-green bot pushed a commit that referenced this pull request Jul 7, 2023
@galz10 galz10 restored the sequence-streaming-fix branch August 2, 2023 20:47
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.

2 participants