fix: bump default version to 1.0.0

[stanislavkozlovski]

This PR fixes #1787. As explained there:

The current default, 0.8.2, is a 5 year-old version. The problem with defaulting to older versions when the Kafka cluster is a newer version is manifold:

• requires Kafka to downconvert consumer response messages (e.g if a newly-configured client produced messages and a default Sarama client is reading) - this has implications on the performance of Kafka, because it uses additional CPU. Especially so if compression is enabled, at which point the broker would need to decompress, downconvert and compress the message back. Downconversion also blocks zerocopy, because your fetch response now needs to be copied into memory
• requires Kafka to upconvert producer requests' messages
• in general is hard to support. While the latest Kafka currently supports the oldest versions, it is in the best interest of the project to deprecate and eventually drop support for legacy versions. Otherwise it becomes hard to maintain, the test matrix grows and new features need to work around old version limitations (no idempotency, exactly once). It is easier for Kafka to deprecate/drop support when its ecosystem has done so already

0.11 is the minimum we should default at as it enables Kafka's v2 message format, avoiding expensive upconversion/downconversion in the Kafka broker. It is also required for correctness in some cases (e.g KIP-101)
1.0.0 is a 3-year old version and I think a good one to default to at this point.

[stanislavkozlovski]

cc @d1egoaz @ijuma @varun06 @twmb

[varun06]

##[error]    async_producer_test.go:170: kafka: client has run out of available brokers to talk to (Is your cluster reachable?)
2205
--- FAIL: TestAsyncProducerMultipleFlushes (90.77s)

hmm, that doesn't look related

[edenhill]

0.11 is the minimum we should default

It should be sufficient to use 0.10 as default and then use ApiVersionRequest to select the highest supported version for each protocol request.

[dnwe]

@edenhill yes the plan is to migrate to ApiVersionRequest in a v2.x release, but I think this is a reasonable default behaviour to leave the 1.x series at so that any user that doesn’t override the version won’t be a hindrance to clusters

[ijuma]

@dnwe +1

[dnwe]

##[error]    async_producer_test.go:170: kafka: client has run out of available brokers to talk to (Is your cluster reachable?)
2205
--- FAIL: TestAsyncProducerMultipleFlushes (90.77s)

hmm, that doesn't look related

It is related, because the mock broker is sending a V0 MetadataResponse when the client (at V1_0_0_0) has sent a V5 MetadataRequest

[stanislavkozlovski]

I'm having a bit of a hard time to fix and pinpoint the issue. I took @dnwe's advice and fixed the test to use V5 MetadataResponses but it still results in a 10-minute timeout with a bunch of threads stuck, apparently when closing the producer and draining the inflight requests.

As a side note - we could benefit from having a single place to define the request version (and we could use this for resp in tests too) - I see we set the metadata request version according to the conf.Version in multiple places in the sender's code. I don't want to increase the scope of this PR but thought I'd note the potential improvement in case we want to capture it in an github issue

[dnwe]

@stanislavkozlovski it'll be making V3 ProducerRequests too, so I think you'll need V3 ProducerResponses

[dnwe]

If you add Logger = log.New(os.Stderr, t.Name(), log.LstdFlags) at the start of the failing test(s) you can see what the mockbroker is doing.

However, as a simpler measure you might want to just update the tests to explicitly specify a Version = MinVersion in their config to just make them continue to exercise the behaviour as they used to. We already have (some) tests that exercise at specific versions.

[stanislavkozlovski]

Thanks @dnwe, I like the approach of switching back the Kafka version as ensuring each request and response are the same version seems like a large task, probably better to be deferred in a separate issue.

The latest commit adds two test functions NewTestConfig in both the mocks package and sarama. Let me know if I should open a GH Issue and mention its number in that function. It'd be good to eventually resolve this as it'll be a gotcha for developers to use NewConfig() in tests and see it fail weirdly

[stanislavkozlovski]

cc @varun06 @dnwe I've got the build passing now. Unfortunately I had to sprinkle a special test config in most tests in order to not default to the new version

[varun06]

@stanislavkozlovski Thanks for getting this through. Use of NewTestConfig seems fine, I wonder if we should add some more documentation around it 🤔

[varun06]

SGTM

[stanislavkozlovski]

Thanks for the review @varun06. I had added documentation around the method:

// NewTestConfig returns a config meant to be used by tests.
// Due to inconsistencies with the request versions the clients send using the default Kafka version
// and the response versions our mocks use, we default to the minimum Kafka version in most tests
func NewTestConfig()

Let me know if you can think of more to say, or whether some other place is best to document it in

[stanislavkozlovski]

Hey there @varun06 @dnwe @bai - what should our next steps be in order to merge this PR?

[varun06]

Need one more approval and then we can merge it.

[dnwe]

LGTM — sorry for the delay