search

heetch / felice

Felice is a nascent, opinionated Kafka library for Go, in Go
MIT License
20 stars 1 forks source link

Fix panicking #42

Closed yaziine closed 5 years ago

yaziine commented 5 years ago

This PR fixes #43.

The first one is in the setup method when trying to retrieve the MaxRetryInterval configuration when c.config is nil.

The second one is involved by fixing the first one: calling setup when c.config is nil won't create the c.retryStrategy. This ends with panic when using it in retry.StartWithCancel. We should create the config before calling setup in the Serve method.

The third one is the panic("unreachable"), when sending SIGINT to the consumer, this piece of code is reachable. We should return a nil Message to close the consumer.

yaziine commented 5 years ago

@rogpeppe it is a fair point and thank you for that.

IIRC the purpose was to have the Consumer's zero value usable but as you said it is very easy to make mistakes because it needs some configuration to be usable.

I propose to create an issue and discuss about it over there.

tealeg commented 5 years ago

This change looks OK, I think, but I'm not entirely sure - ISTM that the initialization code could be a bit clearer.

I don't think it's easy to fix without breaking backward compatibility, but ISTM that the way that Consumer is created and configured makes it very easy to make this kind of mistake. Currently AFAICS you are expected to create an instance of the Consumer yourself and optionally populate some of the fields. Other fields are filled in when you call Consumer.Serve. If there was a single entry constructor function for the Consumer which received a Config value and returned a new ready-to-use Consumer, all the code could rely on having correctly set up configuration.

I don't really understand the distinction between the different kinds of config parameters. Why are Logger and Metrics fields inside the Consumer, not inside Config?. By putting Config as an argument to Serve, we're ensuring that the Handle method can't see any of that configuration, which might not be an issue now, but might be later.

These are excellent points - they probably deserve to be addressed separately. Fancy raising an issue @rogpeppe ? In the context of felice, I think its still safe to make those kinds of breaking changes (especially considering the general approach to dependency management via vendoring in Go projects).

yaziine commented 5 years ago

@rogpeppe @tealeg I created an issue https://github.com/heetch/felice/issues/44