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

gzip filter: allow setting zlib compressor's chunk size #10508

Merged
merged 22 commits into from
Apr 25, 2020
Merged

gzip filter: allow setting zlib compressor's chunk size #10508

merged 22 commits into from
Apr 25, 2020

Conversation

rgs1
Copy link
Member

@rgs1 rgs1 commented Mar 25, 2020

Signed-off-by: Raul Gutierrez Segales rgs@pinterest.com

gzip filter: allow setting zlib compressor's chunk size
6adfd9d 
Signed-off-by: Raul Gutierrez Segales <rgs@pinterest.com>
@rgs1 rgs1 requested a review from dio as a code owner March 25, 2020 00:24
@repokitteh-read-only
Copy link

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to api/.

🐱

Caused by: #10508 was opened by rgs1.

see: more, trace.

@rgs1
Copy link
Member Author

rgs1 commented Mar 25, 2020

fwiw, nginx sets this value to 8kb. see:

http://nginx.org/en/docs/http/ngx_http_gzip_module.html#gzip_buffers

@rgs1 rgs1 mentioned this pull request Mar 25, 2020
Open
Update docs
424fe41 
Signed-off-by: Raul Gutierrez Segales <rgs@pinterest.com>
@rgs1 rgs1 mentioned this pull request Mar 25, 2020
Merged
@rgs1
Copy link
Member Author

rgs1 commented Mar 25, 2020

/retest

@repokitteh-read-only
Copy link

🔨 rebuilding ci/circleci: coverage (failed build)

🐱

Caused by: a #10508 (comment) was created by @rgs1.

see: more, trace.

@dio
Copy link
Member

dio commented Mar 25, 2020 •
edited
Loading

Looks good. For the API review, @lizan or maybe @htuch, could you help with this? Thanks!

htuch
htuch reviewed Mar 26, 2020
View reviewed changes
Copy link
Member

@htuch htuch left a comment

Choose a reason for hiding this comment

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

API generally looks good, just a couple of clarifying questions.

api/envoy/config/filter/http/gzip/v2/gzip.proto Outdated Show resolved Hide resolved
api/envoy/config/filter/http/gzip/v2/gzip.proto Outdated Show resolved Hide resolved
@stale
Copy link

stale bot commented Apr 2, 2020

This pull request has been automatically marked as stale because it has not had activity in the last 7 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

@stale stale bot added stale stalebot believes this issue/PR has not been touched recently and removed stale stalebot believes this issue/PR has not been touched recently labels Apr 2, 2020
Raul Gutierrez Segales added 3 commits April 2, 2020 15:24
Merge remote-tracking branch 'upstream/master' into gzip-filter-allow…
3604115 
…-setting-zlib-next-output-buffer
google.protobuf.UInt64Value --> UInt32Value
3aa1ae3 
Signed-off-by: Raul Gutierrez Segales <rgs@pinterest.com>
proto: fix validation rules
1d3aec3 
Signed-off-by: Raul Gutierrez Segales <rgs@pinterest.com>
@htuch
Copy link
Member

htuch commented Apr 3, 2020

API LGTM, curious how this relates to #10553? CC @rojkov

@rgs1
Copy link
Member Author

rgs1 commented Apr 3, 2020

API LGTM, curious how this relates to #10553? CC @rojkov

We probably want to expose this there as well, but given it'll take a while for the gzip filter to go away we could start here.

@rojkov
Copy link
Member

rojkov commented Apr 3, 2020

This PR looks good. I'm going to extend #10553 when this one is merged.

@htuch
Copy link
Member

htuch commented Apr 3, 2020

/lgtm api

@repokitteh-read-only repokitteh-read-only bot removed the api label Apr 3, 2020
@stale stale bot added the stale stalebot believes this issue/PR has not been touched recently label Apr 11, 2020
Raul Gutierrez Segales added 2 commits April 13, 2020 10:33
Merge remote-tracking branch 'upstream/master' into gzip-filter-allow…
9b5c43a 
…-setting-zlib-next-output-buffer
Delete old version history file
a977e5b 
Signed-off-by: Raul Gutierrez Segales <rgs@pinterest.com>
@stale stale bot removed the stale stalebot believes this issue/PR has not been touched recently label Apr 13, 2020
@dio
Copy link
Member

dio commented Apr 16, 2020

Seems like we need to merge master.

Raul Gutierrez Segales added 3 commits April 16, 2020 13:04
Merge remote-tracking branch 'upstream/master' into gzip-filter-allow…
2b86067 
…-setting-zlib-next-output-buffer
Fix history
88bb09c 
Signed-off-by: Raul Gutierrez Segales <rgs@pinterest.com>
Sync protos
e4b7161 
Signed-off-by: Raul Gutierrez Segales <rgs@pinterest.com>
@rgs1
Copy link
Member Author

rgs1 commented Apr 16, 2020

Unrelated failure:

+ GCOV_PREFIX=/build/tmp/_bazel_bazel/b570b5ccd0454dc9af9f65ab1833764d/sandbox/processwrapper-sandbox/5160/execroot/envoy/_coverage/test/coverage/coverage_tests/shard_27_of_50/test
+ export LLVM_PROFILE_FILE=/build/tmp/_bazel_bazel/b570b5ccd0454dc9af9f65ab1833764d/sandbox/processwrapper-sandbox/5160/execroot/envoy/_coverage/test/coverage/coverage_tests/shard_27_of_50/test/%h-%p-%m.profraw
+ LLVM_PROFILE_FILE=/build/tmp/_bazel_bazel/b570b5ccd0454dc9af9f65ab1833764d/sandbox/processwrapper-sandbox/5160/execroot/envoy/_coverage/test/coverage/coverage_tests/shard_27_of_50/test/%h-%p-%m.profraw
+ [[ ! -z '' ]]
+ cd /build/tmp/_bazel_bazel/b570b5ccd0454dc9af9f65ab1833764d/sandbox/processwrapper-sandbox/5160/execroot/envoy/bazel-out/k8-fastbuild/bin/test/coverage/coverage_tests.runfiles/envoy
+ /build/tmp/_bazel_bazel/b570b5ccd0454dc9af9f65ab1833764d/sandbox/processwrapper-sandbox/5160/execroot/envoy/bazel-out/k8-fastbuild/bin/test/coverage/coverage_tests.runfiles/envoy/test/coverage/coverage_tests --gmock_default_mock_behavior=2 '--log-path /dev/null' '-l trace'
external/bazel_tools/tools/test/collect_coverage.sh: line 131: 30183 Segmentation fault      (core dumped) "$@"
+ TEST_STATUS=139
+ touch /build/tmp/_bazel_bazel/b570b5ccd0454dc9af9f65ab1833764d/sandbox/processwrapper-sandbox/5160/execroot/envoy/bazel-out/k8-fastbuild/testlogs/test/coverage/coverage_tests/shard_27_of_50/coverage.dat
+ [[ 139 -ne 0 ]]
+ echo --

@rgs1
Copy link
Member Author

rgs1 commented Apr 16, 2020

/retest

@repokitteh-read-only
Copy link

🔨 rebuilding ci/circleci: coverage (failed build)

🐱

Caused by: a #10508 (comment) was created by @rgs1.

see: more, trace.

Raul Gutierrez Segales added 2 commits April 20, 2020 11:34
Merge remote-tracking branch 'upstream/master' into gzip-filter-allow…
59a261b 
…-setting-zlib-next-output-buffer
Update version history
4a4d6be 
Signed-off-by: Raul Gutierrez Segales <rgs@pinterest.com>
@htuch
Copy link
Member

htuch commented Apr 24, 2020

/lgtm api

@mattklein123 mattklein123 self-assigned this Apr 24, 2020
mattklein123
mattklein123 requested changes Apr 24, 2020
View reviewed changes
Copy link
Member

@mattklein123 mattklein123 left a comment

Choose a reason for hiding this comment

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

LGTM with small comments. Thank you!

/wait

api/envoy/config/filter/http/gzip/v2/gzip.proto Outdated Show resolved Hide resolved
source/extensions/filters/http/gzip/gzip_filter.cc Outdated Show resolved Hide resolved
Raul Gutierrez Segales added 3 commits April 24, 2020 18:15
Merge remote-tracking branch 'upstream/master' into gzip-filter-allow…
75f640e 
…-setting-zlib-next-output-buffer
Use PROTOBUF_GET_WRAPPED_OR_DEFAULT
391f978 
Signed-off-by: Raul Gutierrez Segales <rgs@pinterest.com>
Revert v2 api changes, we are v3 only now
644b14c 
Signed-off-by: Raul Gutierrez Segales <rgs@pinterest.com>
@rgs1 rgs1 force-pushed the gzip-filter-allow-setting-zlib-next-output-buffer branch from 9ab99a9 to 644b14c Compare April 24, 2020 22:32
@rgs1
Copy link
Member Author

rgs1 commented Apr 24, 2020 •
edited
Loading

@mattklein123 sorry about the force push :-(

I made the mistake of fixing a merge conflict from the UI and forgot to pull back.

Raul Gutierrez Segales added 2 commits April 24, 2020 19:03
Fix
f10da4d 
Signed-off-by: Raul Gutierrez Segales <rgs@pinterest.com>
Fix of the fix
703e368 
Signed-off-by: Raul Gutierrez Segales <rgs@pinterest.com>
mattklein123
mattklein123 approved these changes Apr 24, 2020
View reviewed changes
Copy link
Member

@mattklein123 mattklein123 left a comment

Choose a reason for hiding this comment

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

Thanks!

@mattklein123 mattklein123 merged commit 0b0213f into envoyproxy:master Apr 25, 2020
spenceral added a commit to spenceral/envoy that referenced this pull request Apr 27, 2020
@spenceral
Merge branch 'master' into health-check-transport-match
f485e79 
Signed-off-by: Spencer Lewis <slewis@squareup.com>

* master:
  fault injection: add support for setting gRPC status (envoyproxy#10841)
  tests: tag tests that fail on Windows with fails_on_windows (envoyproxy#10940)
  Fix typo on Postgres Proxy documentation. (envoyproxy#10930)
  fuzz: improve header/data stop/continue modeling in HCM fuzzer. (envoyproxy#10931)
  gzip filter: allow setting zlib compressor's chunk size (envoyproxy#10508)
  http: replace vector/reserve with InlinedVector in codec helper (envoyproxy#10941)
  stats: add utilities to create stats from a vector of tokens, mixing dynamic and symbolic elements. (envoyproxy#10735)
  hcm: avoid invoking 100-continue handling on decode filter. (envoyproxy#10929)
  prometheus stats: Correctly group lines of the same metric name. (envoyproxy#10833)
  status: Fix ASAN error in Status payload handling (envoyproxy#10906)
  path: Fix merge slash for paths ending with slash and present query args (envoyproxy#10922)
  compressor filter: add benchmark (envoyproxy#10464)
  xray: expected_span_name is not captured by the lambda with MSVC (envoyproxy#10934)
  ci: update before purge in cleanup (envoyproxy#10938)
  tracer: Improve test coverage for x-ray (envoyproxy#10890)
  Revert "init: order dynamic resource initialization to make RTDS always be first (envoyproxy#10362)" (envoyproxy#10919)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rojkov rojkov rojkov left review comments

@dio dio dio left review comments

@lizan lizan lizan left review comments

@htuch htuch htuch left review comments

@mattklein123 mattklein123 mattklein123 approved these changes

Assignees

@dio dio

@mattklein123 mattklein123

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@rgs1 @dio @htuch @rojkov @lizan @mattklein123