-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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
compression: create a decompressor extensibility point and move gzip decompressor #10744
Conversation
Signed-off-by: Jose Nino <jnino@lyft.com>
Signed-off-by: Jose Nino <jnino@lyft.com>
Signed-off-by: Jose Nino <jnino@lyft.com>
Signed-off-by: Jose Nino <jnino@lyft.com>
Signed-off-by: Jose Nino <jnino@lyft.com>
…atted Signed-off-by: Jose Nino <jnino@lyft.com>
Signed-off-by: Jose Nino <jnino@lyft.com>
Signed-off-by: Jose Nino <jnino@lyft.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
api/envoy/extensions/compression/common/decompressor/v3/decompressor.proto
Outdated
Show resolved
Hide resolved
@htuch yep, this is the decompression dual to #10533. I got the idea for this proposal while reviewing #10533, and I am suggesting @rojkov updates #10533 to the organization proposed here (while keeping the necessary stuff around for proper deprecation). This proposal basically takes @rojkov's work a bit further and gives compression top-level extension status. My proposal a) allows us to move the current code in |
@junr03 ack, makes sense. I actually prefer the organization that has the |
source/extensions/compression/common/decompressor/factory_base.h
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM overall! Some minor suggestions
api/envoy/extensions/compression/common/decompressor/v3/decompressor.proto
Outdated
Show resolved
Hide resolved
api/envoy/extensions/compression/gzip/decompressor/v3/gzip.proto
Outdated
Show resolved
Hide resolved
source/extensions/compression/common/decompressor/factory_base.h
Outdated
Show resolved
Hide resolved
source/extensions/compression/common/decompressor/factory_base.h
Outdated
Show resolved
Hide resolved
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! |
Signed-off-by: Jose Nino <jnino@lyft.com>
@rojkov, now that your PR is pretty much ready to merge, I have updated this PR for the decompressor side. Do you mind reviewing it? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great! I've found an inconsistency in my own PR even while looking at this one.
Signed-off-by: Jose Nino <jnino@lyft.com>
@rojkov I updated this PR after merging yours on friday. PTAL when you can, thanks! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks great! Just one note about auto-docs.
@rojkov updated! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for fixing the title of compressor’s gzip.proto.
Looks good to me.
Signed-off-by: Jose Nino <jnino@lyft.com>
@mattklein123 this is ready for a pass from you |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice!
Description: this PR creates decompressors as an extension point and moves the zlib based gzip decompressor.
Risk Level: low - moving files around. None of the decompressor files were used in a public API capacity so nothing has to be deprecated.
Testing: all existing unit, integration, fuzz tests work.
Docs Changes: pending
Release Notes: pending
Progress toward #4445. Next PR would use this in a generic decompression filter a la #10553.
Signed-off-by: Jose Nino jnino@lyft.com