-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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 Beats to the current Sarama fork #41655
base: main
Are you sure you want to change the base?
Conversation
This pull request is now in conflicts. Could you fix it? 🙏
|
This pull request does not have a backport label.
To fixup this pull request, you need to add the backport labels for the needed
|
|
Pinging @elastic/elastic-agent-data-plane (Team:Elastic-Agent-Data-Plane) |
qq: Why don't we directly point to the fork i.e., github.com/elastic/sarama@beats-fork? Asking this because we have faced some problem with this before. See the PR description: #39840 (comment). As it was breaking for the apm team, upon investigation, I found the problem and that's why we moved to using the fork directly. If required, I can also send the related Slack conversation in Slack. So basically what'd happen is anyone using beats as a dependency, the replace directive won't work for them. |
I think we can. @faec could you please confirm? Any particular reason we don't directly use the fork |
I don't see a reason we can't, but the reason we don't is probably that we would prefer to revert to upstream someday. Though there admittedly has not been much movement on that, so I don't think that's an overriding concern if it simplifies dependency handling overall. |
go.mod
Outdated
@@ -18,7 +18,7 @@ require ( | |||
github.com/Microsoft/go-winio v0.6.2 | |||
github.com/PaesslerAG/gval v1.2.2 | |||
github.com/PaesslerAG/jsonpath v0.1.1 | |||
github.com/Shopify/sarama v1.27.0 | |||
github.com/Shopify/sarama v1.27.0 // indirect |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where is the indirect dependency coming from here? (Do we pick up both versions in the actual build?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The output of go mod why github.com/Shopify/sarama
says:
# github.com/Shopify/sarama
github.com/elastic/beats/v7/metricbeat/module/kafka/consumergroup
github.com/elastic/beats/v7/metricbeat/module/kafka/consumergroup.test
github.com/bsm/sarama-cluster
github.com/Shopify/sarama
The sarama-cluster
package is only used in a single integration test.
As a side note, you can automate changes in import paths using gofmt -w -r '"github.com/Shopify/sarama" -> "github.com/IBM/sarama"' . |
@lalit-satapathy could we have someone from your team reviewing here please? |
This pull request is now in conflicts. Could you fix it? 🙏
|
@@ -57,7 +57,16 @@ var ( | |||
"2.4": sarama.V2_4_0_0, | |||
"2.5": sarama.V2_5_0_0, | |||
"2.6": sarama.V2_6_0_0, | |||
"2.7": sarama.V2_7_0_0, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
supported versions were referenced from here https://docs.aws.amazon.com/msk/latest/developerguide/supported-kafka-versions.html
This PR updates Beats to the current version of Elastic's Sarama fork. It upgrades sarama version to 1.43.3.
The import path has been changed from
github.com/Shopify/sarama
togithub.com/elastic/sarama
due to change in ownership.