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

Update to sarama v1.10.0 #2190

Merged
merged 2 commits into from
Aug 10, 2016
Merged

Update to sarama v1.10.0 #2190

merged 2 commits into from
Aug 10, 2016

Conversation

urso
Copy link

@urso urso commented Aug 8, 2016

No description provided.

@urso urso added in progress Pull request is currently in progress. libbeat labels Aug 8, 2016
TLS: nil,
Timeout: 30 * time.Second,
Worker: 1,
Metadata: metaConfig{
Copy link
Contributor

Choose a reason for hiding this comment

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

What will happen if these settings are used with an older version of Kafka?

Copy link
Author

Choose a reason for hiding this comment

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

sarama lib supports kafka 0.8 - 0.10 . Settings should be valid for all kafka versions. Lib might internally ignore settings if they do not apply to the version being used. AFAIK metadata updates and publisher settings should have same effect for kafka versions.

@urso urso force-pushed the upd/sarama-v1.10.0 branch 2 times, most recently from 06d4598 to ec5fae2 Compare August 10, 2016 10:00
@urso urso added review and removed in progress Pull request is currently in progress. labels Aug 10, 2016
@@ -108,16 +108,28 @@ func (c *client) AsyncPublishEvents(
topic = event["type"].(string)
}

var ts time.Time
if c.config.Version.IsAtLeast(sarama.V0_10_0_0) {
Copy link
Contributor

Choose a reason for hiding this comment

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

A code comment would be nice that quickly explains why whis is only possible with >= 0.10

Copy link
Author

Choose a reason for hiding this comment

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

done

@ruflin
Copy link
Contributor

ruflin commented Aug 10, 2016

Doesn't this PR also need an update the libbeat config file?

@urso urso force-pushed the upd/sarama-v1.10.0 branch from ec5fae2 to 29d2dc8 Compare August 10, 2016 11:50
@urso
Copy link
Author

urso commented Aug 10, 2016

Doesn't this PR also need an update the libbeat config file?

Ups, yes.

@urso urso force-pushed the upd/sarama-v1.10.0 branch 2 times, most recently from 560dfe8 to 75ba5e9 Compare August 10, 2016 13:33
@urso urso force-pushed the upd/sarama-v1.10.0 branch from 75ba5e9 to 44da82f Compare August 10, 2016 13:49
@urso
Copy link
Author

urso commented Aug 10, 2016

@ruflin added missing config file updates + added options to docs

@andrewkroh andrewkroh merged commit 1851dc8 into elastic:master Aug 10, 2016
@urso urso deleted the upd/sarama-v1.10.0 branch February 19, 2019 18:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants