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

[ISSUE-520] Pool gzip compressor and buffer used in gzip writer #521

Merged
merged 6 commits into from
Apr 11, 2024

Conversation

pakio
Copy link
Contributor

@pakio pakio commented Apr 9, 2024

Description

Changed to pool gzip.Writer object and bytes.Buffer object used for gzip compression, for the better performance.

Issues Resolved

Closes #520

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

@pakio pakio force-pushed the ISSUE-520_pool-gzip-compressor branch 2 times, most recently from 5629a3c to 66b4bf9 Compare April 9, 2024 13:15
@pakio
Copy link
Contributor Author

pakio commented Apr 9, 2024

I will update changelog once the change is finalized.
Also I'm not very sure how can I address the error in the Lint check, it seems like detecting error somewhere I didn't updated.

Copy link
Member

@dblock dblock left a comment

Choose a reason for hiding this comment

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

Looks good. Add to CHANGELOG, + see some asks below. Iterate to green.

// Modifications Copyright OpenSearch Contributors. See
// GitHub history for details.

// Licensed to Elasticsearch B.V. under one or more contributor
Copy link
Member

Choose a reason for hiding this comment

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

Is this code copied from open source Elasticsearch client or is this new? If it's new, you don't need to reproduce the "Licensed to ES" part of the license.

t.Run("gzip multiple times", func(t *testing.T) {
gzipCompressor := newGzipCompressor()
body := `{"query":{"match_all":{}}}`
for i := 0; i < 5; i++ {
Copy link
Member

Choose a reason for hiding this comment

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

Maybe generate a different/random/based on i body every time? Feels like a missed opportunity :)

t.Fatal("expected body to be the same after compressing and decompressing")
}
}
})
Copy link
Member

Choose a reason for hiding this comment

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

If the compressor is a noop and just returns the same string, these tests pass.

  1. Add a test that compressed data is different and smaller than the original data.
  2. Add a test that decompressing a random buffer returns the expected errors.
  3. Add a test that compresses data, then compresses it again, and decompresses it twice to get to source, ensuring nothing is lost.

@dblock
Copy link
Member

dblock commented Apr 10, 2024

I will update changelog once the change is finalized. Also I'm not very sure how can I address the error in the Lint check, it seems like detecting error somewhere I didn't updated.

Rebase, we may have fixed this elsewhere since the main build is green.

@pakio pakio force-pushed the ISSUE-520_pool-gzip-compressor branch from 66b4bf9 to fe6bd68 Compare April 11, 2024 11:19
Signed-off-by: Kazuma (Pakio) Arimura <k.arimura96@gmail.com>
Copy link

codecov bot commented Apr 11, 2024

Codecov Report

Attention: Patch coverage is 83.87097% with 5 lines in your changes are missing coverage. Please review.

Project coverage is 66.21%. Comparing base (06a6dc8) to head (0d39036).
Report is 6 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #521      +/-   ##
==========================================
+ Coverage   57.29%   66.21%   +8.92%     
==========================================
  Files         315      364      +49     
  Lines        9823     8587    -1236     
==========================================
+ Hits         5628     5686      +58     
+ Misses       2902     1589    -1313     
- Partials     1293     1312      +19     
Flag Coverage Δ
integration 59.53% <83.87%> (+8.69%) ⬆️
unit 81.66% <83.87%> (+68.82%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Coverage Δ
opensearchtransport/opensearchtransport.go 86.57% <83.33%> (+1.41%) ⬆️
opensearchtransport/gzip.go 84.00% <84.00%> (ø)

... and 249 files with indirect coverage changes

@pakio pakio force-pushed the ISSUE-520_pool-gzip-compressor branch 2 times, most recently from d40fcd7 to a0851ed Compare April 11, 2024 12:00
Signed-off-by: Kazuma (Pakio) Arimura <k.arimura96@gmail.com>
Signed-off-by: Kazuma (Pakio) Arimura <k.arimura96@gmail.com>
@pakio pakio force-pushed the ISSUE-520_pool-gzip-compressor branch from a0851ed to bc5b63d Compare April 11, 2024 12:02
Signed-off-by: Kazuma (Pakio) Arimura <k.arimura96@gmail.com>
Signed-off-by: Kazuma (Pakio) Arimura <k.arimura96@gmail.com>
Signed-off-by: Kazuma (Pakio) Arimura <k.arimura96@gmail.com>
@dblock dblock merged commit 7ea16f7 into opensearch-project:main Apr 11, 2024
53 checks passed
@pakio
Copy link
Contributor Author

pakio commented Apr 11, 2024

oh I was about to send some message regarding the change. thanks you @dblock for your review and merging the PR!

@dblock
Copy link
Member

dblock commented Apr 11, 2024

Good work @pakio, thank you!

@pakio pakio deleted the ISSUE-520_pool-gzip-compressor branch April 12, 2024 00:15
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.

[FEATURE] Use pooled gzip writer for request body gzip compression
2 participants