Skip to content
This repository has been archived by the owner on Jun 23, 2023. It is now read-only.

Import consumer from Kafka-go #18

Merged
merged 14 commits into from
Oct 29, 2018
Merged

Import consumer from Kafka-go #18

merged 14 commits into from
Oct 29, 2018

Conversation

yaziine
Copy link

@yaziine yaziine commented Oct 19, 2018

Fix #10.

Notes:

  • the logging and metrics have been removed
  • new MetricsHook interface that can be passed to receive metrics

@yaziine yaziine requested review from tealeg and asdine October 19, 2018 09:23
Copy link
Contributor

@tealeg tealeg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We still need to move consumer.Stop from Kafka-go as a well.

Copy link
Contributor

@tealeg tealeg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

"github.com/pkg/errors"
)

// Consumer is a Kafka consumer.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🌷 this isn't a great comment, but I'll pick this up in the documentation work.

}
}

func (c *Consumer) convertMessage(cm *sarama.ConsumerMessage) *message.Message {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🌷 this is the sort of thing that we might want to pull out of the Consumer type and make public if we want people to be able to user felice in a modular way.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For example, if they want to do some specific task/changes in the message?

Copy link
Contributor

@asdine asdine left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The consumer lacks logs, especially debug logs describing the lifecycle of the goroutines, consumption attempts etc. Even though some of these will be written in Kafka-Go, I'm not sure it will be enough.

Copy link
Contributor

@tealeg tealeg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we add logging we should do it via an interface. It's probably OK to expect a structured logger (after all you could write an adapter to use a non-structured one if you really wish). Perhaps this is something for a separate branch?

@asdine
Copy link
Contributor

asdine commented Oct 24, 2018

I'm not sure we should go as far as abstracting logging: This is an opinionated library and IMO this kind of library can choose how it logs (whether by using the std logger or a third party library) and simply allow the user choose the output and the level if any; Sarama and a lot of other libraries already do that.
Logging the right information at the right place is a feature provided by the library and not having to abstract that would allow us to be more flexible.

@yaziine
Copy link
Author

yaziine commented Oct 24, 2018

Maybe we can do as Sarama does. By default, it logs to ioutil.Discard but if the user wants to retrieve the logs, he has to set the Logger variable to redirect the output where he wants. WDYT guys?

@asdine
Copy link
Contributor

asdine commented Oct 24, 2018

Something like that would be cool yeah, a library shouldn't print anything by default.

@yaziine
Copy link
Author

yaziine commented Oct 24, 2018

a library shouldn't print anything by default

It's for why I want to remove the loggers because it's not common (based on my experience) to have library logs as a project will do (think of regula for example).
But the way of Sarama is good I think. Let's wait for @tealeg input.

@asdine
Copy link
Contributor

asdine commented Oct 24, 2018

It's for why I want to remove the loggers because it's not common (based on my experience) to have library logs as a project will do (think of regula for example).

Not printing by default and not providing logs are two different things. It's important for a library to log when it's doing internal complex stuff, regardless of the output, and it is the user's choice to display these logs or not.

@yaziine
Copy link
Author

yaziine commented Oct 24, 2018

For me (until these days) a library didn't have to log. If something goes wrong, it just returns errors and the users will manage that with logs and so on. But now it's more clear for me.

@tealeg
Copy link
Contributor

tealeg commented Oct 24, 2018

I like the ioutil.Discard approach - let's do that.

@yaziine yaziine added the WIP label Oct 24, 2018
consumer/consumer.go Outdated Show resolved Hide resolved
consumer/consumer.go Outdated Show resolved Hide resolved

func (c *Consumer) setup() {
if c.handlers == nil {
c.handlers = &handler.Collection{}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🌷 What is the reasoning behind having c.handlers as a pointer since all we do is instantiate it like this? Shouldn't we simply use a value field instead of a pointer field?

@yaziine yaziine merged commit 55e75bd into master Oct 29, 2018
@yaziine yaziine deleted the import-consumer branch October 29, 2018 14:23
@tealeg tealeg mentioned this pull request Nov 2, 2018
tealeg pushed a commit that referenced this pull request Nov 13, 2018
Import consumer from Kafka-go
tealeg pushed a commit that referenced this pull request Nov 13, 2018
Import consumer from Kafka-go
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants