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.

Sign up for GitHub

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

Jump to bottom

feat(internal): Add zstd to internal content_coding #13423

Merged
merged 15 commits into from
Jun 22, 2023
Merged

feat(internal): Add zstd to internal content_coding #13423

merged 15 commits into from
Jun 22, 2023

Conversation

zekena2
Copy link
Contributor

@zekena2 zekena2 commented Jun 12, 2023

Required for all PRs

helps #12875

@telegraf-tiger telegraf-tiger bot added the feat Improvement on an existing feature such as adding a new setting/mode to an existing plugin label Jun 12, 2023
srebhan
srebhan approved these changes Jun 12, 2023
View reviewed changes
Copy link
Contributor

@srebhan srebhan 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 to me. Thanks for this contribution @zekena2!

@srebhan srebhan added the ready for final review This pull request has been reviewed and/or tested by multiple users and is ready for a final review. label Jun 12, 2023
@srebhan srebhan changed the title feat(internal): add zstd to internal content_coding feat(internal): Add zstd to internal content_coding Jun 12, 2023
Hipska
Hipska suggested changes Jun 12, 2023
View reviewed changes
internal/content_coding.go Outdated Show resolved Hide resolved
Hipska
Hipska reviewed Jun 12, 2023
View reviewed changes
internal/content_coding.go Outdated Show resolved Hide resolved
@srebhan
Copy link
Contributor

srebhan commented Jun 12, 2023

Honestly speaking, we should fix the max-size later in the correct way i.e. by using the decoder option. @Hipska you are now trying to enforce something that was mandated by a linter to a place where it does not serve any purpose besides artificially creating an error...

@Hipska
Copy link
Contributor

Hipska commented Jun 12, 2023

I Indeed have no idea, why this check is there and why it must be an error when some decoded data is some size, but now at least the Zstd decoder is behaving exactly the same as others. Uniformity is more important to me.

Or do you suggest to change this behaviour for all decoders in this PR as well?

@zekena2
Copy link
Contributor Author

zekena2 commented Jun 12, 2023

@srebhan Is this implementation correct?

Hipska
Hipska suggested changes Jun 13, 2023
View reviewed changes
internal/content_coding.go Outdated Show resolved Hide resolved
internal/content_coding.go Outdated Show resolved Hide resolved
plugins/inputs/cloud_pubsub/cloud_pubsub.go Outdated Show resolved Hide resolved
plugins/inputs/cloud_pubsub/cloud_pubsub.go Outdated Show resolved Hide resolved
srebhan
srebhan requested changes Jun 13, 2023
View reviewed changes
Copy link
Contributor

@srebhan srebhan left a comment

Choose a reason for hiding this comment

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

Please revert the changes to IdentityDecoder and simply ignore the options there. Otherwise the PR looks good.

internal/content_coding.go Outdated Show resolved Hide resolved
@powersj powersj removed their assignment Jun 14, 2023
@powersj powersj removed the ready for final review This pull request has been reviewed and/or tested by multiple users and is ready for a final review. label Jun 14, 2023
srebhan
srebhan reviewed Jun 15, 2023
View reviewed changes
Copy link
Contributor

@srebhan srebhan left a comment

Choose a reason for hiding this comment

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

Thanks for fixing the identity part @zekena2! Some more comments, especially regarding setting the new option only if an actual size was specified by the user and otherwise use the default...

plugins/inputs/amqp_consumer/amqp_consumer.go Outdated Show resolved Hide resolved
plugins/inputs/amqp_consumer/amqp_consumer_test.go Outdated Show resolved Hide resolved
plugins/inputs/cloud_pubsub/cloud_pubsub.go Outdated Show resolved Hide resolved
internal/content_coding.go Outdated Show resolved Hide resolved
zekena2
zekena2 commented Jun 21, 2023
View reviewed changes
internal/content_coding.go Outdated Show resolved Hide resolved
zekena2
zekena2 commented Jun 21, 2023
View reviewed changes
internal/content_coding.go Outdated Show resolved Hide resolved
zekena2
zekena2 commented Jun 21, 2023
View reviewed changes
internal/content_coding.go Outdated Show resolved Hide resolved
srebhan and others added 2 commits June 21, 2023 15:18
@srebhan @zekena2
Update internal/content_coding.go
7444123 
Co-authored-by: Zeyad Kenawi <zeyad.kenawi@starship.co>
@zekena2
revert: use the equivalent zstd levels
6be9aab
@srebhan srebhan added the ready for final review This pull request has been reviewed and/or tested by multiple users and is ready for a final review. label Jun 22, 2023
Hipska
Hipska suggested changes Jun 22, 2023
View reviewed changes
Copy link
Contributor

@Hipska Hipska 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 to me! Only a minor remark..

internal/content_coding.go Outdated Show resolved Hide resolved
@telegraf-tiger
Copy link
Contributor

Download PR build artifacts for linux_amd64.tar.gz, darwin_amd64.tar.gz, and windows_amd64.zip.
Downloads for additional architectures and packages are available below.

☺️ This pull request doesn't significantly change the Telegraf binary size (less than 1%)

📦 Click here to get additional PR build artifacts

Artifact URLs

DEB RPM TAR GZ ZIP
amd64.deb aarch64.rpm darwin_amd64.tar.gz windows_amd64.zip
arm64.deb armel.rpm darwin_arm64.tar.gz windows_arm64.zip
armel.deb armv6hl.rpm freebsd_amd64.tar.gz windows_i386.zip
armhf.deb i386.rpm freebsd_armv7.tar.gz
i386.deb ppc64le.rpm freebsd_i386.tar.gz
mips.deb riscv64.rpm linux_amd64.tar.gz
mipsel.deb s390x.rpm linux_arm64.tar.gz
ppc64el.deb x86_64.rpm linux_armel.tar.gz
riscv64.deb linux_armhf.tar.gz
s390x.deb linux_i386.tar.gz
linux_mips.tar.gz
linux_mipsel.tar.gz
linux_ppc64le.tar.gz
linux_riscv64.tar.gz
linux_s390x.tar.gz

@srebhan srebhan removed their assignment Jun 22, 2023
@powersj powersj merged commit 577db89 into influxdata:master Jun 22, 2023
24 checks passed
powersj pushed a commit to powersj/telegraf that referenced this pull request Jul 5, 2023
@zekena2 @powersj
feat(internal): Add zstd to internal content_coding (influxdata#13423)
2489a68
@srebhan srebhan added this to the v1.28.0 milestone Sep 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Hipska Hipska Hipska approved these changes

@powersj powersj powersj approved these changes

@srebhan srebhan srebhan approved these changes

Assignees

@powersj powersj

Labels
feat Improvement on an existing feature such as adding a new setting/mode to an existing plugin ready for final review This pull request has been reviewed and/or tested by multiple users and is ready for a final review.
Projects
None yet
Milestone
v1.28.0
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@zekena2 @srebhan @Hipska @powersj