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

use a large enough buffer to flush in #3067

Merged
merged 7 commits into from
May 11, 2023
Merged

use a large enough buffer to flush in #3067

merged 7 commits into from
May 11, 2023

Conversation

Geal
Copy link
Contributor

@Geal Geal commented May 9, 2023

When writing a deferred response, if the output buffer was too small to write the entire compressed response, the compressor would write a small chunk, that did not decompress to the entire primary response, and would then wait for the next response to send the rest. Unfortunately, we cannot really know the output size we need in advance, and if we asked the decoder, it will tell us that it flushed all the data, even if it could have sent more.
So in here we raise the output buffer size, and do a second buffer growth step after flushing if necessary.

Checklist

Complete the checklist (and note appropriate exceptions) before a final PR is raised.

  • Changes are compatible[^1]
  • Documentation[^2] completed
  • Performance impact assessed and acceptable
  • Tests added and passing[^3]
    • Unit Tests
    • Integration Tests
    • Manual Tests

Exceptions

Note any exceptions here

Notes

[^1]. It may be appropriate to bring upcoming changes to the attention of other (impacted) groups. Please endeavour to do this before seeking PR approval. The mechanism for doing this will vary considerably, so use your judgement as to how and when to do this.
[^2]. Configuration is an important part of many changes. Where applicable please try to document configuration examples.
[^3]. Tick whichever testing boxes are applicable. If you are adding Manual Tests:
- please document the manual testing (extensively) in the Exceptions.
- please raise a separate issue to automate the test and label it (or ask for it to be labeled) as manual test

@github-actions

This comment has been minimized.

@router-perf
Copy link

router-perf bot commented May 9, 2023

CI performance tests

  • pr - Basic stress test that finds the max RPS when no features are configured
  • reload - Reload test over a long period of time

@Geal Geal marked this pull request as ready for review May 10, 2023 12:08
@Geal Geal requested review from garypen and o0Ignition0o May 10, 2023 12:08
Copy link
Contributor

@bnjjj bnjjj left a comment

Choose a reason for hiding this comment

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

Just delete the println and we're good

apollo-router/src/axum_factory/axum_http_server_factory.rs Outdated Show resolved Hide resolved
apollo-router/src/axum_factory/axum_http_server_factory.rs Outdated Show resolved Hide resolved
apollo-router/src/axum_factory/compression/mod.rs Outdated Show resolved Hide resolved
Co-authored-by: Jeremy Lempereur <jeremy.lempereur@iomentum.com>
@Geal Geal merged commit ecfb997 into dev May 11, 2023
@Geal Geal deleted the geal/resize-flushed-buffer branch May 11, 2023 09:00
@abernix abernix mentioned this pull request May 11, 2023
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