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

feat: flush writes to http response on every chunk #743

Merged
merged 4 commits into from
Oct 4, 2023
Merged

feat: flush writes to http response on every chunk #743

merged 4 commits into from
Oct 4, 2023

Conversation

itayariel
Copy link
Contributor

@itayariel itayariel commented Jul 25, 2023 •
edited

This pr enables streaming responses using the proxy code
Currently the logging middleware breaks streaming and responses are sent only upon completion

Checklist

Fixes #742

@itayariel itayariel requested a review from a team as a code owner July 25, 2023 08:22
JorTurFer
JorTurFer reviewed Jul 26, 2023
View reviewed changes
Copy link
Member

@JorTurFer JorTurFer left a comment

Choose a reason for hiding this comment

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

Nice improvement! ❤️
Could you introduce an entry in changelog and any test to cover this scenario? If this is coverable with unit test it'd be nice, if not, we also execute e2e test on each PR/commit.

There is also a problem with DCO, could you sign your commits please? it's a requirement for us

@itayariel
Copy link
Contributor Author

Thanks will work on your comments

JorTurFer
JorTurFer reviewed Sep 4, 2023
View reviewed changes
Copy link
Member

@JorTurFer JorTurFer left a comment

Choose a reason for hiding this comment

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

Could you add a unit test for this?

CHANGELOG.md Outdated
@@ -34,6 +34,7 @@ This changelog keeps track of work items that have been completed and are ready
- **Operator**: Remove ScaledObject `name` & `app` custom labels ([#717](https://github.com/kedacore/http-add-on/issues/717))
- **Scaler**: Provide graceful shutdown for grpc server on SIGINT and SIGTERM ([#731](https://github.com/kedacore/http-add-on/issues/731))
- **Scaler**: remplement custom interceptor metrics ([#718](https://github.com/kedacore/http-add-on/issues/718))
- **Interceptor**: Add support for streaming responses ([#726](https://github.com/kedacore/http-add-on/issues/743))
Copy link
Member

Choose a reason for hiding this comment

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

Please, sort this alphabetically and update the id too

Suggested change
- **Interceptor**: Add support for streaming responses ([#726](https://github.com/kedacore/http-add-on/issues/743))
- **Interceptor**: Add support for streaming responses ([#742](https://github.com/kedacore/http-add-on/issues/https://github.com/kedacore/http-add-on/issues/742))

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

@JorTurFer
Copy link
Member

Hi @itayariel
We will cut a release probably this month (we are coming back from the summer break and we are still a bit slow). Do you have any update about this?

@itayariel
Copy link
Contributor Author

hey @JorTurFer I tried to write unit tests, but didn't a find nice a way to test the fix, do you have any suggestion?

JorTurFer
JorTurFer reviewed Sep 17, 2023
View reviewed changes
CHANGELOG.md Outdated
@@ -34,7 +34,7 @@ This changelog keeps track of work items that have been completed and are ready
- **Operator**: Remove ScaledObject `name` & `app` custom labels ([#717](https://github.com/kedacore/http-add-on/issues/717))
- **Scaler**: Provide graceful shutdown for grpc server on SIGINT and SIGTERM ([#731](https://github.com/kedacore/http-add-on/issues/731))
- **Scaler**: remplement custom interceptor metrics ([#718](https://github.com/kedacore/http-add-on/issues/718))
- **Interceptor**: Add support for streaming responses ([#726](https://github.com/kedacore/http-add-on/issues/743))
- **Interceptor**: Add support for streaming responses ([#743](https://github.com/kedacore/http-add-on/issues/742))
Copy link
Member

@JorTurFer JorTurFer Sep 17, 2023 •
edited

Choose a reason for hiding this comment

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

Suggested change
- **Interceptor**: Add support for streaming responses ([#743](https://github.com/kedacore/http-add-on/issues/742))
- **Interceptor**: Add support for streaming responses ([#742](https://github.com/kedacore/http-add-on/issues/742))

@JorTurFer
Copy link
Member

hey @JorTurFer I tried to write unit tests, but didn't a find nice a way to test the fix, do you have any suggestion?

The recorder used for the logginResponseWritter unit tests has a flag to know if flush has been called Flushed. I guess that thanks to this check in unit tests, and the e2e tests, we can have at least a minimum guarantee but if you could write an e2e test for this feature would be even better.

@JorTurFer
Copy link
Member

any update on this?

@itayariel @JorTurFer
feat: flush writes to http response on every chunk
2bf3ba1 
Signed-off-by: Itay Ariel <itay@cnvrg.io>
Signed-off-by: Jorge Turrado <jorge.turrado@scrm.lidl>
@itayariel @JorTurFer
chor: add changelog
2a62403 
Signed-off-by: Jorge Turrado <jorge.turrado@scrm.lidl>
@itayariel @JorTurFer
chor: update docs
4d344f3 
Signed-off-by: Jorge Turrado <jorge.turrado@scrm.lidl>
@JorTurFer JorTurFer enabled auto-merge (squash) October 4, 2023 19:12
@JorTurFer
fix changelog
43da6ba 
Signed-off-by: Jorge Turrado <jorge.turrado@scrm.lidl>
@JorTurFer JorTurFer enabled auto-merge (squash) October 4, 2023 19:36
@JorTurFer JorTurFer merged commit 0a1d1de into kedacore:main Oct 4, 2023
21 checks passed
@JorTurFer JorTurFer mentioned this pull request Feb 14, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@JorTurFer JorTurFer JorTurFer approved these changes

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

Successfully merging this pull request may close these issues.

Keda Http Add-on support streaming
2 participants
@itayariel @JorTurFer