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

Switch to fork of sarama without zstd dependency #6349

Merged
merged 1 commit into from
Sep 5, 2019
Merged

Conversation

danielnelson
Copy link
Contributor

@danielnelson danielnelson commented Sep 4, 2019

The newer sarama library uses CGO for it's zstd compression. Telegraf does not support enabling zstd compression currently, so it is safe to remove. This avoids the build requiring GLIBC_2.14, but we may want to make further changes to the build to avoid linking newer symbol versions.

Changes against sarama 1.23.1: https://github.com/influxdata/sarama/commit/b12709e6ca29240128c89fe0b30b6a76be42b457

closes #6344

Required for all PRs:

  • Signed CLA.
  • Associated README.md updated.
  • Has appropriate unit tests.

@danielnelson danielnelson added area/packaging fix pr to fix corresponding bug labels Sep 4, 2019
@danielnelson danielnelson added this to the 1.12.1 milestone Sep 4, 2019
@danielnelson danielnelson merged commit 11d40a9 into master Sep 5, 2019
@danielnelson danielnelson deleted the glibc-version branch September 5, 2019 02:05
danielnelson added a commit that referenced this pull request Sep 5, 2019
bitcharmer pushed a commit to bitcharmer/telegraf that referenced this pull request Oct 18, 2019
@andig
Copy link

andig commented Oct 31, 2019

Unfortunately, that commit b12709e6ca29240128c89fe0b30b6a76be42b457 does not seem to exist. Currently that breaks moving to go modules.

Should use latest commit https://github.com/influxdata/sarama/commit/d6c39e51b1aa805d23878ebe101a30675dc17866 instead?

@glinton
Copy link
Contributor

glinton commented Oct 31, 2019

Looks like it exists on the without-zstd branch - https://github.com/influxdata/sarama/commit/b12709e6ca29240128c89fe0b30b6a76be42b457 if that helps

@andig
Copy link

andig commented Nov 1, 2019

Looks like it exists on the without-zstd branch - influxdata/sarama@b12709e if that helps

Got it- for some reason github search doesn't find that commit, opened a ticket.

Working on #6118 I still can't get modules to work for sarama (posting here as the forked sarama repo doesn't support issues). My go.mod has:

replace github.com/Shopify/sarama v1.19.0 => github.com/influxdata/sarama v1.23.2-0.20190904202538-b12709e6ca29

yet:

❯ go run cmd/telegraf/telegraf.go -debug -sample-config
go: github.com/influxdata/sarama@v1.23.2-0.20190904202538-b12709e6ca29: parsing go.mod:
        module declares its path as: github.com/Shopify/sarama
                but was required as: github.com/influxdata/sarama

I think that the fork needs to have an updated module name in go.mod that matches its url.

Does that make sense? The I'd be happy to open a PR.

athoune pushed a commit to bearstech/telegraf that referenced this pull request Apr 17, 2020
idohalevi pushed a commit to idohalevi/telegraf that referenced this pull request Sep 29, 2020
arstercz pushed a commit to arstercz/telegraf that referenced this pull request Mar 5, 2023
arstercz pushed a commit to arstercz/telegraf that referenced this pull request Mar 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/packaging fix pr to fix corresponding bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

telegraf 1.12 debian wheezy package : depend on GLIBC_2.14
3 participants