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

Refactoring #244 - Comments robustness #250

Conversation

dellagustin
Copy link
Contributor

This commit is a refactoring on the Comments function to improve robustness for loading partial responses form the comments API.

Once we introduced partial responses (i.e. chunked encoding), it worked in dev, but failed in production due to buffering in the reverse proxy (nginx).

This was already solved with
#247, but only if the reverse proxy remains unchanged.

With this refactoring, the implementation would continue to work even if buffering took place, making it more robust.

For testing, the change introduced in the PR mentioned above was reverted (temporarily).

For additinal testing instructions, see
#247 (comment)

This commit is a refactoring on the Comments function to improve
robustness for loading partial responses form the comments API.

Once we introduced partial responses (i.e. chunked encoding),
it worked in dev, but failed in production due to buffering in
the reverse proxy (nginx).

This was already solved with
Podcastindex-org#247, but only if
the reverse proxy remains unchanged.

With this refactoring, the implementation would continue to work
even if buffering took place, making it more robust.

For testing, the change introduced in the PR mentioned above was
reverted (temporarily).

For additinal testing instructions, see
Podcastindex-org#247 (comment)
@dellagustin
Copy link
Contributor Author

@ericpp , you may want to take a look at this PR.

@ericpp
Copy link
Contributor

ericpp commented Apr 13, 2023

@dellagustin I can take a look at it. Here was my approach before finding the Nginx header: ericpp@2ffe5c4

@daveajones daveajones merged commit de423ff into Podcastindex-org:master Apr 14, 2023
@ericpp
Copy link
Contributor

ericpp commented Apr 14, 2023

@dellagustin Looks good to me. I tested it out with 512, 1k, and 8k Nginx buffer sizes and they all worked great!

@dellagustin
Copy link
Contributor Author

@dellagustin Looks good to me. I tested it out with 512, 1k, and 8k Nginx buffer sizes and they all worked great!

Great, thanks!

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.

3 participants