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:support zstd compress and uncompressed #1701

Merged
merged 8 commits into from
Feb 21, 2024

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.

go.mod Outdated Show resolved Hide resolved
@@ -150,7 +151,7 @@ func nonblockingWriteZstd(ctxv any) {

// 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