downgrade sarama to v1.45.2

[dkrotx]

Reverts #7222 except docker/docker-compose-async-wf-kafka-v4.yml
then switches Shopify/sarama -> IBM/sarama v1.45.2

This way it doesn't bring -deprecated packages into go.mod (like we had with the latest, IBM/sarama v1.46)

This was done because 1.46 looks like it is introducing some significant changes - possibly subtle internal and breaking changes and is very recently released (hopefully they'll have some patches soon if they're actually indeed causing internal breaks)

[dkrotx]

@ansidev could you check if the previous sarama release works for you? https://github.com/IBM/sarama/releases/tag/v1.45.2
I think it does not bring -deprecated packages.

[ansidev]

@ansidev could you check if the previous sarama release works for you? https://github.com/IBM/sarama/releases/tag/v1.45.2 I think it does not bring -deprecated packages.

As I have mentioned in #7222 (comment), it seems that

golang.org/x/tools/go/expect v0.1.1-deprecated // indirect

has been required because of github.com/uber/cadence/bench/load/basic.

I downgraded sarama to v1.45.2, then ran make pr and the deprecated package still remains in go.mod.

[dkrotx]

I see. Thanks for checking this!
At the same time, I don't quite understand how is that possible - we see the -deprecated packages in go.mod appeared in #7222 . Was the github.com/uber/cadence/bench/load/basic dependency introduce there?

update: just read #7222 (comment) and I think github.com/uber/cadence/bench/load/basic relies on go/expect. Isn't then the sarama update caused expect package to switch?

I downgraded sarama to v1.45.2, then ran make pr and the deprecated package still remains in go.mod.

Do you know if downgrading is equal to taking the previous revision, and applying [email protected] instead of the latest?

[dkrotx]

Just checked, v1.45.2 does not bring -deprecated. This happens only after switching to v1.46:

$ go get github.com/IBM/[email protected]
go: upgraded github.com/IBM/sarama v1.45.2 => v1.46.0
...
go: upgraded golang.org/x/tools v0.22.0 => v0.35.0


$ go mod tidy
go: finding module for package golang.org/x/tools/go/expect
go: downloading golang.org/x/tools v0.37.0
go: found golang.org/x/tools/go/expect in golang.org/x/tools/go/expect v0.1.1-deprecated <<<<<

[davidporter-id-au]

@ansidev re the deprecated dependenc:

golang.org/x/tools/go/expect

as far as I can tell the go mod why is slightly misleading in this instance, I don't think(?) it's used in the github.com/uber/cadence/bench/load/basic package, else I'd just remove it. It's used in static tooling in the RPC libraries as a transitive dev dependency. unfortunately removing it is going to be a pain, but hopefully is something that those RPC libraries can get on. I'm not super sure why the go mod why provides an explanation like that, but it might be just showing the first hit.

Regarding 1.46.0 vs 1.45.2, I am noticing that there is some some internal changes and problems caused (unit test failure only so far, not yet confirmed with actual production traffic), but I'll keep the thread updated once I learn more.

[puellanivis]

The “landmine” seems to have been primed with honnef.co/go/[email protected] which just so happened to add an import to golang.org/x/tools/go/expect. (All subsequent versions of the honnef.co/go/tools/unused have kept this import.)

As long as all the other packages never depended on golang.org/x/tools from v0.35.0 or after, then golang.org/x/tools module contained a golang.org/x/tools/go/expect package, and so go mod tidy never had to go looking for a specific sub-module to meet the dependency, but once we update any of the golang.org/x/... packages to a version that requires v0.35.0 or later, suddenly now, the module doesn’t have the path, and it looks for a sub-module with the path, and it ends up pulling in a *-deprecated branch.

So, here, it was not actually the update of sarama itself, but of golang.org/x/[email protected] to golang.org/x/[email protected] that triggers this, (you can test this by attempting to update this package alone… or just update the golang.org/x/tools package directly) but the underlying issue is that honnef.co/go/tools/unused from v0.32.0 onwards is depending on the deprecated-and-now-removed package. And it just so happens to not be in any of the diffs, because the code was fine, until the MVS (minimal version selection) bumps the golang.org/x/tools package at least one release too far.

[davidporter-id-au]

The “landmine” seems to have been primed with honnef.co/go/[email protected] which just so happened to add an import to golang.org/x/tools/go/expect. (All subsequent versions of the honnef.co/go/tools/unused have kept this import.)

As long as all the other packages never depended on golang.org/x/tools from v0.35.0 or after, then golang.org/x/tools module contained a golang.org/x/tools/go/expect package, and so go mod tidy never had to go looking for a specific sub-module to meet the dependency, but once we update any of the golang.org/x/... packages to a version that requires v0.35.0 or later, suddenly now, the module doesn’t have the path, and it looks for a sub-module with the path, and it ends up pulling in a *-deprecated branch.

So, here, it was not actually the update of sarama itself, but of golang.org/x/[email protected] to golang.org/x/[email protected] that triggers this, (you can test this by attempting to update this package alone… or just update the golang.org/x/tools package directly) but the underlying issue is that honnef.co/go/tools/unused from v0.32.0 onwards is depending on the deprecated-and-now-removed package. And it just so happens to not be in any of the diffs, because the code was fine, until the MVS (minimal version selection) bumps the golang.org/x/tools package at least one release too far.

hey, appreciate your taking a look at this and the fast response, I did a quick dig around and saw that expect was being pulled in through honnef.co/go/tools and I think this was due to our reliance on staticcheck, that seemed... scary-looking but not an actual issue (afaik).

However, I saw a very large amount of unit tests break when we pulled it in to a large go monorepo, forcing probably a few days of investigation at the very least. The Unit test failures were very much centred around kafka message publishing behaviour, but since they were from a team like 5 layers away from where I am in the stack I didn't spend a huge amount of time determining if they were just terrible tests which had managed to rely on internal implementation details or a genuine issue. Once I have a little more time next week I can probably dig around and try and see.

For now, just due to the likely load on some service-owners we just bumped it back a version and will triage what's going on with 1.46.2 once we have slightly less time pressure