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

feat: Use klauspost/compress for gzip #14

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Conversation

li-jin-gou
Copy link
Collaborator

@li-jin-gou li-jin-gou commented Oct 29, 2023

What type of PR is this?

feat

What this PR does / why we need it (English/Chinese):

  • zh: 使用 klauspost/compress 代替标准库
  • en: use klauspost/compress instead of standard

Which issue(s) this PR fixes:

@li-jin-gou li-jin-gou force-pushed the optimize/gzip branch 3 times, most recently from d72ad35 to 5f304cb Compare November 12, 2023 14:48
@li-jin-gou
Copy link
Collaborator Author

Hello @klauspost,I copied https://github.com/cloudwego/hertz/tree/develop/pkg/common/compress over and changed the gzip dependency, but the original single test was not used. Hope you can help me look at the unit test when you have time, thank you very much.

@klauspost
Copy link

@li-jin-gou The test seems to expect the gzip package to produce a specific compressed output. That is a wrong expectation.

While you can change the expected output, it will break again. So either A) check the decompressed output or B) Check that the size isn't 5% bigger than you expect. Exact compressed size will change over time as the algorithm is improved.

@li-jin-gou
Copy link
Collaborator Author

The test seems to expect the gzip package to produce a specific compressed output. That is a wrong expectation.

While you can change the expected output, it will break again. So either A) check the decompressed output or B) Check that the size isn't 5% bigger than you expect. Exact compressed size will change over time as the algorithm is improved.

Thanks

gzip_test.go Show resolved Hide resolved
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