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

Breaking API change in a minor version release (v1.34.0) #2358

Closed
Jeffail opened this issue Oct 6, 2022 · 2 comments · Fixed by #2494
Closed

Breaking API change in a minor version release (v1.34.0) #2358

Jeffail opened this issue Oct 6, 2022 · 2 comments · Fixed by #2494

Comments

@Jeffail
Copy link

Jeffail commented Oct 6, 2022

Problem Description

Hey, we recently upgraded sarama from v1.30.1 to v1.37 in https://github.com/benthosdev/benthos. However, in v1.34.0 you introduced a breaking API change to (*OffsetCommitRequest) AddBlock (59a3d39#diff-df5fe5e6335379dccdd93c2bf7f3e52783a9b16cad64e53383066f5d8d2b070cR223). This might not seem like a big deal but because you're using Go modules and you haven't incremented the major version number it means downstream importers can potentially conflict with each other on which signature they're using, making it impossible(?) to import them both at the same time.

For example, a hypothetical user might want to import both benthos v3 and v4 at the same time, if v4 is using a version of sarama after v1.34 then their build will break because both imports will differ as to which signature they expect. I'm not sure how feasible it is for that user to fix things downstream.

We're going to have to downgrade for now, is this something you'd be willing to remedy in a future release?

@dnwe
Copy link
Collaborator

dnwe commented Oct 11, 2022

@Jeffail thanks for getting in touch. That's interesting, I wasn't aware that anyone was driving the offset commit protocol directly via the broker.go call rather than via offset_manager.go (or via a consumer client)

We can look at restoring the old AddBlock in another point release to fix this up in the short term

@mihaitodor
Copy link

I took a quick peek across GitHub and multiple projects will be impacted by this breaking change when they'll have to upgrade Sarama. It can be a nightmare if it's a transitive dependency and it would help a lot if you can address this somehow @dnwe before any major projects start patching their code to work around this change.

dnwe added a commit that referenced this issue Jul 17, 2023
In v1.34.0 a breaking API change to (*OffsetCommitRequest) AddBlock was
inadvertently introduced by 59a3d39
We weren't aware that anyone was driving the offset commit protocol
directly via the broker.go call rather than via offset_manager.go (or
via a consumer client)

For now we restore the old AddBlock signature and move the new one to
AddBlockWithLeaderEpoch. This will unfortunately impact anyone who had
updated their own code to call the new func signature, but it's probably
more important that we restore the longer term backwards compatibility
now until we move to a v2 release.

Fixes #2358
@dnwe dnwe closed this as completed in 4b9e8f6 Jul 17, 2023
ioanzicu pushed a commit to ioanzicu/sarama that referenced this issue Jul 31, 2023
In v1.34.0 a breaking API change to (*OffsetCommitRequest) AddBlock was
inadvertently introduced by 59a3d39
We weren't aware that anyone was driving the offset commit protocol
directly via the broker.go call rather than via offset_manager.go (or
via a consumer client)

For now we restore the old AddBlock signature and move the new one to
AddBlockWithLeaderEpoch. This will unfortunately impact anyone who had
updated their own code to call the new func signature, but it's probably
more important that we restore the longer term backwards compatibility
now until we move to a v2 release.

Fixes IBM#2358

Signed-off-by: Dominic Evans <dominic.evans@uk.ibm.com>
Signed-off-by: Ioan Zicu <ioan.zicu@nokia.com>
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 a pull request may close this issue.

3 participants