-
Notifications
You must be signed in to change notification settings - Fork 917
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
[OE] Support compression for requests #6366
base: main
Are you sure you want to change the base?
[OE] Support compression for requests #6366
Conversation
fdc842b
to
218f752
Compare
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
❌ Invalid Prefix For Manual Changeset CreationInvalid description prefix. Found "feat". Only "skip" entry option is permitted for manual commit of changeset files. If you were trying to skip the changelog entry, please use the "skip" entry option in the ##Changelog section of your PR description. |
007f616
to
0b166e8
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #6366 +/- ##
==========================================
- Coverage 67.70% 67.69% -0.01%
==========================================
Files 3417 3417
Lines 66922 66928 +6
Branches 10888 10891 +3
==========================================
Hits 45310 45310
- Misses 18966 18968 +2
- Partials 2646 2650 +4
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
? DEFAULT_COMPRESSION | ||
: typeof rawConfig.compression === 'string' | ||
? rawConfig.compression | ||
: undefined; |
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.
An expression of rawConfig.compression = false
gets out of this as undefined
which is the wrong message out of this function (because it was defined). For the sanity of future devs, we should:
: undefined; | |
: false; |
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.
Also, for the sanity of future devs, can we not use nested ternaries, especially without parens? calling a function with readable if statements isn't the worst thing imo :)
@AMoo-Miki @ashwin-pc could you help take another look at this PR which is targeting 2.15? |
@ashwin-pc, I think Rocky is tied up. If you agree with my suggestion, we can move this along. |
@@ -120,6 +121,10 @@ export function parseClientOptions(config: OpenSearchClientConfig, scoped: boole | |||
clientOptions.disablePrototypePoisoningProtection = config.disablePrototypePoisoningProtection; | |||
} | |||
|
|||
if (config.compression) { | |||
clientOptions.compression = config.compression; |
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.
do we want to add a test for this given we updated the test to not do this by default?
? DEFAULT_COMPRESSION | ||
: typeof rawConfig.compression === 'string' | ||
? rawConfig.compression | ||
: undefined; |
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.
Also, for the sanity of future devs, can we not use nested ternaries, especially without parens? calling a function with readable if statements isn't the worst thing imo :)
move to 2.16 |
Description
Support compressing traffic with
opensearch.compression: true
oropensearch.compress: 'gzip'
.Also, set compression in the Node server and OpenSearch traffic for CI.
Issues Resolved
#5296
Changelog
opensearch.compression: true
oropensearch.compress: 'gzip'
Check List
yarn test:jest
yarn test:jest_integration