feat: make ErrOutOfBrokers wrap the underlying error that prevented connections to the brokers

[k-wall]

With this change, the user of Sarama is required to use Go 1.13's errors.Is etc (rather then ==) when forming conditionals returned by this library.

[bai]

@k-wall Thanks for your contribution! Looks like there are some linter failures on Go 1.17, could you please take a look?

[k-wall]

@bai thanks for looking.

I'm not certain what the output from the Github Action is trying to tell me. I think you are referring to these errors in the output.

run golangci-lint
  Running [/home/runner/golangci-lint-1.44.0-linux-amd64/golangci-lint run --out-format=github-actions] in [] ...
  Error: undeclared name: `errors` (typecheck)
  Error: undeclared name: `errors` (typecheck)
  Error: undeclared name: `errors` (typecheck)
  Error: undeclared name: `errors` (typecheck)
  Error: undeclared name: `errors` (typecheck)
  Error: undeclared name: `errors` (typecheck)
  Error: undeclared name: `errors` (typecheck)
  Error: undeclared name: `errors` (typecheck)
  Error: undeclared name: `errors` (typecheck)
  Error: undeclared name: `errors` (typecheck)

However, when I run the tooling locally against my branch, I see a different set of errors.

My local tools:

% go version
go version go1.17.6 darwin/amd64
% golangci-lint version
golangci-lint has version 1.44.0 built from 617470f on 2022-01-25T11:22:07Z

and the output I get when I run the CI command line locally:

golangci-lint run --out-format=github-actions
::error file=metrics_test.go,line=103,col=6::`minCountMeterValidator` is unused (deadcode)
::error file=metrics_test.go,line=134,col=6::`minCountHistogramValidator` is unused (deadcode)
::error file=metrics_test.go,line=156,col=6::`minValHistogramValidator` is unused (deadcode)
::error file=metrics_test.go,line=165,col=6::`maxValHistogramValidator` is unused (deadcode)
::error file=metrics_test.go,line=60,col=28::func `(*metricValidators).registerForGlobalAndTopic` is unused (unused)

Can you give me a pointer to help diagnose the problem or tell me how to get verbose output from CI? My best guess is that the tool believes that the new import { errors } is somehow conflicts with existing fields on the same name of structs like responsePromise???

[dnwe]

@k-wall the functional tests are gated behind a build flag so to lint the full source code directly you need to set that flag before running golangci-lint (or you can use the makefile version which does it for you via make lint):

GOFLAGS="-tags=functional" golangci-lint
functional_client_test.go:23:6: undeclared name: `errors` (typecheck)
        if !errors.Is(err, ErrOutOfBrokers) {
            ^
functional_client_test.go:40:54: undeclared name: `errors` (typecheck)
        if err := client.RefreshMetadata("unknown_topic"); !errors.Is(err, ErrUnknownTopicOrPartition) {
                                                            ^
functional_client_test.go:44:51: undeclared name: `errors` (typecheck)
        if _, err := client.Leader("unknown_topic", 0); !errors.Is(err, ErrUnknownTopicOrPartition) {
                                                         ^
functional_client_test.go:48:53: undeclared name: `errors` (typecheck)
        if _, err := client.Replicas("invalid/topic", 0); !errors.Is(err, ErrUnknownTopicOrPartition) && !errors.Is(err, ErrInvalidTopic) {
                                                           ^
functional_consumer_group_test.go:532:40: undeclared name: `errors` (typecheck)
                if err := m.Consume(ctx, topics, m); errors.Is(err, ErrClosedConsumerGroup) {
                                                     ^
functional_consumer_test.go:27:61: undeclared name: `errors` (typecheck)
        if _, err := consumer.ConsumePartition("test.1", 0, -10); !errors.Is(err, ErrOffsetOutOfRange) {
                                                                   ^
functional_consumer_test.go:31:71: undeclared name: `errors` (typecheck)
        if _, err := consumer.ConsumePartition("test.1", 0, math.MaxInt64); !errors.Is(err, ErrOffsetOutOfRange) {
                                                                             ^
functional_producer_test.go:100:78: undeclared name: `errors` (typecheck)
        if _, _, err := producer.SendMessage(&ProducerMessage{Topic: "in/valid"}); !errors.Is(err, ErrUnknownTopicOrPartition) && !errors.Is(err, ErrInvalidTopic) {
                                                                                    ^
functional_test.go:388:9: undeclared name: `errors` (typecheck)
        return errors.Is(err, ErrUnknownTopicOrPartition) || errors.Is(err, ErrInvalidTopic) || errors.Is(err, ErrNoError)
               ^
functional_test.go:392:9: undeclared name: `errors` (typecheck)
        return errors.Is(err, ErrTopicAlreadyExists) || errors.Is(err, ErrNoError)

These files are just missing the "errors" import entirely — you can fix them with GOFLAGS="-tags=functional" goimports -w *.go

[dnwe]

I've made those changes, rebased the commits and force pushed to get a build through — I'll go ahead and review now

[dnwe]

@k-wall do we need to also change the type switches to if errors.As(err, ...) as well?

[k-wall]

I'm having second thoughts about the wisdom of the errors.Is refactor within the implementation. My intent was to help make clear that API now requires the sarama user must use errors.Is etc. I thought adopting that within the implementation that would reinforce the practice. There's a couple of things that don't sit well:

• there a runtime cost to errors.Is (reflection). - kafka is supposed to be high performance - and this detracts from that.
• expressions involving errors.IsI(err, NoError) just seem weird. I worry about a NoError getting wrapped causing a weird bug.
• it's turned out to be more intrusive than I anticipated.

I thinking of an alternative that limit errors.Is use on the public API boundaries (i.e the interfaces). I think this might just boil down to the tests using errors.Is and new documentation on Client, AsyncProducer etc. The implementation would be reverted back.

Thanks again for the time you are giving me.

[k-wall]

@dnwe hi - my last message wasn't too clear, I was trying to ask WDYT to the suggestion?

[k-wall]

In the end, retaining the current errors.Is/As and refactoring the type switches seemed like the right approach. I've also enabled the errorlint linter.

[dnwe]

@k-wall thanks! will take a look

[k-wall]

Thanks for the feedback, I'll update the PR based in the feedback.

I'd also like to ask about the behaviour of Client#InitProducerID(). I think it behaves in a different way to the other parts of the API that return ErrOutOfBrokers (RefreshMetadata etc). If I understand its current behaviour, in the event all brokers becoming unconnectable (say) it will return the error returned by the last broker tried. I think the only time it can return a ErrOutOfBrokers result is if client.any() returns nil on first attempt (which I'm not sure can ever actually happen).

Is there a reason why it error contract is different? I am wondering if it should actually return ErrOutOfBrokers, and with this change, wrap the errors from the brokers?

[dnwe]

@k-wall yes I agree, it would make sense for the overall error ErrOutOfBrokers to be returned from InitProducerID too and the per-broker errors wrapped

[dnwe]

@k-wall thanks for all your hard work on this — merged 🎉