Skip to content

feat:support zstd compress and uncompressed#1701

Merged
erikdubbelboer merged 8 commits intovalyala:masterfrom
Max-Cheng:feat/zstd_compress
Feb 21, 2024
Merged

feat:support zstd compress and uncompressed#1701
erikdubbelboer merged 8 commits intovalyala:masterfrom
Max-Cheng:feat/zstd_compress

Conversation

@Max-Cheng
Copy link
Contributor

No description provided.

@Max-Cheng Max-Cheng marked this pull request as ready for review January 31, 2024 14:07
Copy link
Collaborator

@erikdubbelboer erikdubbelboer left a comment

Choose a reason for hiding this comment

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

Tests are failing.

@Max-Cheng
Copy link
Contributor Author

Tests are failing.

Cause compress is declaring go.mod' with go 1.19'. In this case, I'll downgrade the version.

@erikdubbelboer
Copy link
Collaborator

Still failing.

@Max-Cheng
Copy link
Contributor Author

Still failing.

Ok. Looks like 1.18 encoding/binary does not implement AppendByteOrder.
1.18
1.19

I will fix this in other ways.

@gaby
Copy link
Contributor

gaby commented Feb 3, 2024

Compress minimum Go version is 1.19, downgrading is not a good idea since it will not have the corruption fixes made to zstd recently.

@erikdubbelboer Any timeline for fasthttp to bump the min go version?

@erikdubbelboer
Copy link
Collaborator

I'm ok with you dropping 1.18 support in this pull. It's not officially maintained by the Go team either.

// AppendZstdBytes appends zstd src to dst and returns the resulting dst.
func AppendZstdBytes(dst, src []byte) []byte {
return AppendZstdBytesLevel(dst, src, CompressBrotliDefaultCompression)
return AppendZstdBytesLevel(dst, src, CompressZstdDefault)
Copy link
Contributor

Choose a reason for hiding this comment

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

Good catch!

Copy link
Collaborator

@erikdubbelboer erikdubbelboer left a comment

Choose a reason for hiding this comment

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

There are still linting errors and it needs to be rebased on master now.

@Max-Cheng
Copy link
Contributor Author

Max-Cheng commented Feb 11, 2024

Please Hold. I'll fix this wrong commiter by rebase

@Max-Cheng
Copy link
Contributor Author

Max-Cheng commented Feb 12, 2024

@erikdubbelboer fixed,need squash those commit?

@gaby
Copy link
Contributor

gaby commented Feb 13, 2024

@erikdubbelboer The timeouts in the tests are too agressive. The same error happens in a different PR

https://github.com/valyala/fasthttp/actions/runs/7881870997/job/21506194694?pr=1720

@gaby
Copy link
Contributor

gaby commented Feb 20, 2024

@alexandear @erikdubbelboer @Max-Cheng Can someone rebase with master to see what's left on this PR?

@erikdubbelboer erikdubbelboer force-pushed the feat/zstd_compress branch 2 times, most recently from 95ac420 to 1d133df Compare February 21, 2024 05:23
@gaby
Copy link
Contributor

gaby commented Feb 21, 2024

@erikdubbelboer If you add this to the lint workflow before the job, it will show you here on Github where the issues are:

permissions:
  # Required: allow read access to the content for analysis.
  contents: read
  # Optional: allow read access to pull request. Use with `only-new-issues` option.
  pull-requests: read
  # Optional: Allow write access to checks to allow the action to annotate code in the PR.
  checks: write

Example: https://github.com/gofiber/fiber/blob/main/.github/workflows/linter.yml

@erikdubbelboer
Copy link
Collaborator

@gaby thanks for the suggestion. I think everything is fixed now and this can be merged.

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