Ingester Consumer #942

davit-y · 2018-07-19T04:38:45Z

Which problem is this PR solving?

Kafka Ingester #929

Short description of the changes

Export consumer to be initialized and started in a main file
Add ConsumerBuilder to pkg/kafka/config
Split pkg/kafka/config into Consumer and Producer

codecov · 2018-07-20T16:08:16Z

Codecov Report

Merging #942 into master will not change coverage.
The diff coverage is 100%.

@@          Coverage Diff          @@
##           master   #942   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files         138    138           
  Lines        6343   6362   +19     
=====================================
+ Hits         6343   6362   +19

Impacted Files	Coverage Δ
cmd/ingester/app/consumer/consumer_metrics.go	`100% <100%> (ø)`	⬆️
cmd/ingester/app/consumer/processor_factory.go	`100% <100%> (ø)`	⬆️
cmd/ingester/app/consumer/consumer.go	`100% <100%> (ø)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8f7e497...10eda27. Read the comment docs.

black-adder · 2018-07-23T16:34:12Z

cmd/ingester/app/consumer/consumer.go

+	config.ConsumerBuilder
+}
+
+// Consumer uses sarama to consume messages from kafka and handle


this sentence doesn't sound right

black-adder · 2018-07-23T16:48:55Z

pkg/kafka/config/config.go

-type Configuration struct {
-	Brokers []string
+// Consumer is an interface to features of Sarama that are necessary for the consumer
+type Consumer interface {


I'm not a big fan of having the interface definition inside the config file. Having said that, I'm not a big fan of it's original location either. Keep this here for now but try to come up with a better location to define this.

black-adder · 2018-07-23T16:49:37Z

pkg/kafka/config/config.go

@@ -15,22 +15,52 @@
 package config


can we split this file into consumer.go and producer.go?

black-adder · 2018-07-23T16:49:58Z

pkg/kafka/config/config.go

 }

 // ProducerBuilder builds a new kafka producer
 type ProducerBuilder interface {
 	NewProducer() (sarama.AsyncProducer, error)
 }

+// ConsumerBuilder builds a new kafka producer


s/producer/consumer

black-adder · 2018-07-23T16:52:48Z

pkg/kafka/config/config.go

+	Brokers []string
+	Topic   string
+	GroupID string
+	Consumer


maybe I'm missing something but I don't see need to embed the Consumer interface in this config

black-adder · 2018-07-23T16:56:30Z

cmd/ingester/app/consumer/options.go

+)
+
+const (
+	configPrefix      = "ingester-consumer"


do we need this variable? I know all our other configs follow this format but I don't see a reason to do so here. Do you foresee a need for us to consume from different kafka topics? This might be more a question for Prithvi

black-adder · 2018-07-23T17:03:15Z

cmd/ingester/app/consumer/consumer.go

-	io.Closer
+// New is a constructor for a Consumer
+func New(params Params) (*Consumer, error) {
+	saramaConsumer, err := params.ConsumerBuilder.NewConsumer()


does params.NewConsumer() not work?

black-adder · 2018-07-23T18:02:02Z

pkg/kafka/config/consumer/config.go

@@ -0,0 +1,48 @@
+// Copyright (c) 2018 The Jaeger Authors.


mb, I meant pkg/kafka/config/consumer.go and pkg/kafka/config/producer.go

actually, keep as is, I prefer producer.Configuration over config.ProducerConfiguration

Sounds good. I actually did it the first way, then noticed that it would be inconsistent with ES and Cassandra and changed it.

I don't think we need the path of this file to be pkg/kafka/config/consumer/config.go; having this be pkg/kafka/consumer/config.go conveys the same information

I'll make the change once the producer PR #957 is merged

vprithvi · 2018-07-25T15:22:29Z

pkg/kafka/config/producer/config.go


 import (
 	"github.com/Shopify/sarama"
 )

+// Builder builds a new kafka producer


Why move this from 26-30 to here? (I feel that this unnecessarily makes the code review bigger)

vprithvi · 2018-07-25T15:24:42Z

plugin/storage/kafka/factory.go

 	producer   sarama.AsyncProducer
 	marshaller Marshaller
+	producer.Builder


Are the producer changes required for this commit?
Could they be better factored into a separate commit?

Won had commented about splitting the config file into consumer and producer packages, would it be better to put everything into config.go in this PR and creating a new PR for the split?

vprithvi · 2018-07-26T17:54:41Z

cmd/ingester/app/consumer/consumer.go

+}
+
+// Close closes the Consumer and underlying sarama consumer
+func (c *Consumer) Close() error {


Any particular reason for moving this?

in the producer PR you recommended this folder structure

Oh, I mean moving Close from L106-L110 to here.

vprithvi · 2018-07-26T17:56:27Z

cmd/ingester/app/consumer/consumer.go

+	ProcessorFactory ProcessorFactory
+	Factory          metrics.Factory
+	Logger           *zap.Logger
+	SaramaConsumer   consumer.Consumer


Seeing that this consumer is an interface that we control, perhaps internalConsumer is an apt variable name

vprithvi · 2018-07-26T17:59:29Z

cmd/ingester/app/consumer/processor_factory.go

 	metricsFactory metrics.Factory
 	logger         *zap.Logger
 	baseProcessor  processor.SpanProcessor
 	parallelism    int
 }

-func (c *processorFactory) new(partition int32, minOffset int64) processor.SpanProcessor {
+// NewFactory constructs a new ProcessorFactory
+func NewFactory(params FactoryParams) (ProcessorFactory, error) {


Return a pointer to ProcessorFactor instead. I'd rename this to NewProcessorFactory, and do the same for L30

Signed-off-by: Davit Yeghshatyan <davo@uber.com>

Removed to reduce the scope of this PR. Will be added once producer and consumer are merged. Signed-off-by: Davit Yeghshatyan <davo@uber.com>

Signed-off-by: Davit Yeghshatyan <davo@uber.com>

davit-y requested review from black-adder, jpkrohling, pavolloffay, vprithvi and yurishkuro as code owners July 19, 2018 04:38

davit-y mentioned this pull request Jul 19, 2018

Kafka Ingester #929

Closed

9 tasks

davit-y changed the title ~~Ingester Consumer~~ [WIP] Ingester Consumer Jul 19, 2018

davit-y force-pushed the ingester-consumer branch from 2f2d3a2 to b5716d7 Compare July 19, 2018 16:40

davit-y changed the title ~~[WIP] Ingester Consumer~~ Ingester Consumer Jul 19, 2018

davit-y force-pushed the ingester-consumer branch from b5716d7 to 548097c Compare July 19, 2018 17:23

vprithvi force-pushed the add-ingester branch from 3622393 to f78da53 Compare July 19, 2018 17:29

davit-y force-pushed the ingester-consumer branch from 548097c to 8d73c56 Compare July 20, 2018 15:41

davit-y changed the base branch from add-ingester to master July 20, 2018 15:42

davit-y force-pushed the ingester-consumer branch from 8d73c56 to 346630d Compare July 20, 2018 15:51

davit-y force-pushed the ingester-consumer branch from ce5ea8a to 3ddb0ce Compare July 20, 2018 21:21

black-adder reviewed Jul 23, 2018

View reviewed changes

davit-y force-pushed the ingester-consumer branch from e61f47c to d1a1ca0 Compare July 23, 2018 17:17

black-adder reviewed Jul 23, 2018

View reviewed changes

davit-y force-pushed the ingester-consumer branch from 86cd708 to 73bb2ea Compare July 23, 2018 18:12

davit-y mentioned this pull request Jul 24, 2018

Ingester Main #952

Merged

davit-y force-pushed the ingester-consumer branch 2 times, most recently from f9330f3 to 53b46cf Compare July 25, 2018 05:57

vprithvi reviewed Jul 25, 2018

View reviewed changes

davit-y mentioned this pull request Jul 25, 2018

Add producer folder to pkg/kafka/config #957

Merged

davit-y force-pushed the ingester-consumer branch from 53b46cf to 0f74051 Compare July 26, 2018 17:36

vprithvi reviewed Jul 26, 2018

View reviewed changes

Davit Yeghshatyan added 2 commits July 26, 2018 14:27

Export consumer

8607bc7

Signed-off-by: Davit Yeghshatyan <davo@uber.com>

Configure consumer with viper

ca26378

Signed-off-by: Davit Yeghshatyan <davo@uber.com>

Davit Yeghshatyan added 10 commits July 26, 2018 14:27

Add kafka ConsumerBuilder

6c12667

Signed-off-by: Davit Yeghshatyan <davo@uber.com>

Remove ProcessorFactory initialization from Consumer

b8d4194

Signed-off-by: Davit Yeghshatyan <davo@uber.com>

Move options from consumer to ingester

e4a5a40

Signed-off-by: Davit Yeghshatyan <davo@uber.com>

Consumer minor fixes

aa58856

Signed-off-by: Davit Yeghshatyan <davo@uber.com>

Split kafka config into consumer and producer

f52ab9b

Signed-off-by: Davit Yeghshatyan <davo@uber.com>

Add test for ProcessorFactory

9d6a4d7

Signed-off-by: Davit Yeghshatyan <davo@uber.com>

Remove Options

ef09c37

Removed to reduce the scope of this PR. Will be added once producer and consumer are merged. Signed-off-by: Davit Yeghshatyan <davo@uber.com>

Take SaramaConsumer as parameter to Consumer

dd63ebd

Signed-off-by: Davit Yeghshatyan <davo@uber.com>

Move config consumer folder

63b6772

Signed-off-by: Davit Yeghshatyan <davo@uber.com>

Rename structs

10eda27

Signed-off-by: Davit Yeghshatyan <davo@uber.com>

davit-y force-pushed the ingester-consumer branch from 29817a1 to 10eda27 Compare July 26, 2018 18:27

vprithvi approved these changes Jul 26, 2018

View reviewed changes

vprithvi merged commit d1c2da6 into jaegertracing:master Jul 26, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Ingester Consumer #942

Ingester Consumer #942

davit-y commented Jul 19, 2018 •

edited

Loading

codecov bot commented Jul 20, 2018 •

edited

Loading

black-adder Jul 23, 2018

black-adder Jul 23, 2018

black-adder Jul 23, 2018

black-adder Jul 23, 2018

black-adder Jul 23, 2018

black-adder Jul 23, 2018

black-adder Jul 23, 2018

black-adder Jul 23, 2018

black-adder Jul 23, 2018

davit-y Jul 23, 2018

vprithvi Jul 25, 2018

davit-y Jul 25, 2018

vprithvi Jul 25, 2018

vprithvi Jul 25, 2018

davit-y Jul 25, 2018

vprithvi Jul 26, 2018

davit-y Jul 26, 2018 •

edited

Loading

vprithvi Jul 26, 2018

vprithvi Jul 26, 2018

vprithvi Jul 26, 2018

Ingester Consumer #942

Ingester Consumer #942

Conversation

davit-y commented Jul 19, 2018 • edited Loading

Which problem is this PR solving?

Short description of the changes

codecov bot commented Jul 20, 2018 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davit-y Jul 26, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davit-y commented Jul 19, 2018 •

edited

Loading

codecov bot commented Jul 20, 2018 •

edited

Loading

davit-y Jul 26, 2018 •

edited

Loading