Ability to use multiple storage types #880

davit-y · 2018-06-22T15:58:40Z

Which problem is this PR solving?

Resolves Kafka for trace storage #274
Continuation of Add chained storage writer #872

Short description of the changes

Adding ability to specify multiple storage types
All listed storage types are used for writing
In the case of multiple storage types, the first is used for Reading and by the ArchiveSpanWriter

codecov · 2018-06-22T16:28:30Z

Codecov Report

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

@@          Coverage Diff          @@
##           master   #880   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files         126    126           
  Lines        6053   6068   +15     
=====================================
+ Hits         6053   6068   +15

Impacted Files	Coverage Δ
plugin/storage/factory.go	`100% <100%> (ø)`	⬆️
plugin/storage/factory_config.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 3884950...71434f4. Read the comment docs.

yurishkuro · 2018-06-22T16:55:53Z

plugin/storage/factory.go

@@ -151,9 +162,13 @@ func (f *Factory) CreateArchiveSpanReader() (spanstore.Reader, error) {

 // CreateArchiveSpanWriter implements storage.ArchiveFactory
 func (f *Factory) CreateArchiveSpanWriter() (spanstore.Writer, error) {
-	factory, ok := f.factories[f.SpanStorageType]
+	if len(f.SpanWriterTypes) > 1 {
+		fmt.Fprintf(f.log, "WARNING: Since there are multiple span storage types,"+


factory should have access to logger. Don't use printf.

yurishkuro · 2018-06-22T16:56:34Z

plugin/storage/factory_config.go

 	SpanStorageTypeEnvVar = "SPAN_STORAGE_TYPE"

+	// SpanReaderTypeEnvVar is the optional env var that defines the type of backend used for span reading in the case of multi-storage.
+	SpanReaderTypeEnvVar = "SPAN_READER_TYPE"


have you tried without introducing this new config? What's not working without it?

Since we can't have a CompositeReader, in the case of multiple storage types we must decide on 1 storage to read from. At the moment, the first listed storage type is used and a warning is emitted, but the extra flag allows users to specify the storage so that they don't have to be concerned with the order of their inputted storage types.

but the extra flag allows users to specify the storage so that they don't have to be concerned with the order of their inputted storage types.

I suggest removing this optional behavior - the same can be achieved by reordering items in the main storage type var without additional code.

the same can be achieved by reordering items in the main storage type var without additional code.

I'm not a fan of this non user friendly behavior where the user has to know the ordering of the flags, the new variable makes it explicit.

I can argue it's simpler to reorder values in one env vars then pass two env vars. The flag description can explain it. Not to mention that this whole scenario is only applicable to all-in-one, which nobody should be running with real storage anyway. So I am 👎 on adding extra complication to the code for this marginal use case (the diff size would probably be 1/2 of what it's now without this).

gonna be a pedant and not forward this conversation in anyway; the ingester will read from kafka and write to kafka/cassandra, this won't be just a marginal use case. As a user, I'd prefer a second explicit flag.

fair point. What about topic names in this case? It sounds like we'd need separate configuration for reader / writer. In theory this always could've been the case even with other storages (write to one cluster/keyspace, read from another), but in case of Kafka this is more obvious.

So if you want to go with extra variable, then it should be symmetric, reader AND writer. Same for dependencies. It sounds like we need a whole overhaul of the storage configurations.

yurishkuro · 2018-06-22T16:56:59Z

plugin/storage/factory_config.go

 	DependenciesStorageType string
+	log                     io.Writer


?

we use *zap.Logger for logging.

yurishkuro · 2018-06-23T02:20:24Z

plugin/storage/factory_config.go

-			token,
-			SpanStorageTypeEnvVar,
-		)
+		logger.Warn("found deprecated command line option " + token +


sorry, I didn't realize we already use this io.Writer. Let's continue using it instead of creating redundant logger in all mains.

jpkrohling · 2018-06-28T14:58:09Z

plugin/storage/factory_config.go

+		fmt.Fprintf(log, "WARNING: Since multiple storage types have been listed,"+
+			"the first span storage type listed (%s) will be used for reading and archiving.", spanWriterTypes[0])
+	}
+	depStorageType := os.Getenv(DependencyStorageTypeEnvVar)


Should this come from viper?

All the main.go files initialize viper after setting up this initial factory.

Indeed, but sounds odd... @yurishkuro, @black-adder, is there a reason why both the main storage and the strategy storage factories are initialized before viper? They are consumed only way after...

If the factories can be created within the Cobra's command, it can also receive a zap.Logger, avoiding the usage of fmt.Fprintf

The reason is to avoid each storage type adding it's flags to cli

That affects only the contents of --help, right?

vprithvi · 2018-06-28T15:15:21Z

plugin/storage/factory_config.go

-		depStoreType = spanStorageType
+	spanWriterTypes := strings.Split(spanStorageType, ",")
+	if len(spanWriterTypes) > 1 {
+		fmt.Fprintf(log, "WARNING: Since multiple storage types have been listed,"+


It seems like this warning could be easily missed.
Is it difficult to require the user to explicitly specify a read and archive storage type when multiple storage types are specified?

we discussed this offline and decided to defer this to later changes when the ingester is implemented. We would like to merge this s that we can publish a new release with Kafka writer support (just to allow people to do data mining).

yurishkuro · 2018-06-28T16:22:54Z

plugin/storage/factory.go

 	}
-	return factory.CreateSpanWriter()
+	return spanstore.NewCompositeWriter(writers...), nil


do we want to short-circuit here for len(writers)==1 by returning the one underlying writer? Composite writer adds perf and cognitive overhead when it's not needed.

yurishkuro · 2018-06-28T16:25:09Z

plugin/storage/factory_config.go

-		depStoreType = spanStorageType
+	spanWriterTypes := strings.Split(spanStorageType, ",")
+	if len(spanWriterTypes) > 1 {
+		fmt.Fprintf(log, "WARNING: Since multiple storage types have been listed,"+


we discussed this offline and decided to defer this to later changes when the ingester is implemented. We would like to merge this s that we can publish a new release with Kafka writer support (just to allow people to do data mining).

yurishkuro · 2018-06-28T16:25:33Z

plugin/storage/factory_config.go

+		fmt.Fprintf(log, "WARNING: Since multiple storage types have been listed,"+
+			"the first span storage type listed (%s) will be used for reading and archiving.", spanWriterTypes[0])
+	}
+	depStorageType := os.Getenv(DependencyStorageTypeEnvVar)


yurishkuro · 2018-06-28T16:26:24Z

plugin/storage/factory_config.go

-		SpanStorageType:         spanStorageType,
-		DependenciesStorageType: depStoreType,
+		SpanWriterTypes:         spanWriterTypes,
+		SpanReaderType:          spanWriterTypes[0],


add // TODO support explicit configuration for readers - maybe book an issue & describe our offline discussions so far.

Signed-off-by: Won Jun Jang <wjang@uber.com>

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

This reverts commit 57dd2a4 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 June 22, 2018 15:58

davit-y changed the title ~~Multi storage~~ Ability to Use Multiple Storage Types Jun 22, 2018

davit-y changed the title ~~Ability to Use Multiple Storage Types~~ Ability to use multiple storage types Jun 22, 2018

davit-y force-pushed the multi-storage branch from e153394 to eeeb790 Compare June 22, 2018 16:28

yurishkuro reviewed Jun 22, 2018

View reviewed changes

yurishkuro reviewed Jun 23, 2018

View reviewed changes

davit-y force-pushed the multi-storage branch from a7495f6 to 75811a6 Compare June 27, 2018 20:33

This was referenced Jun 28, 2018

Kafka for trace storage #274

Closed

Add chained storage writer #872

Closed

jpkrohling reviewed Jun 28, 2018

View reviewed changes

vprithvi reviewed Jun 28, 2018

View reviewed changes

yurishkuro approved these changes Jun 28, 2018

View reviewed changes

black-adder and others added 7 commits June 28, 2018 17:22

Add chained storage writer

668bca4

Signed-off-by: Won Jun Jang <wjang@uber.com>

remove unecessary slice

fb7253d

Signed-off-by: Won Jun Jang <wjang@uber.com>

Use composite writer and add reader flag

eec275a

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

Add logger to factory

a021f2d

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

Revert "Add logger to factory"

d5d4a7a

This reverts commit 57dd2a4 Signed-off-by: Davit Yeghshatyan <davo@uber.com>

Remove SpanReaderType environment variable

2bfe385

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

Only use CompositeWriter for multiple writers

345cd41

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

davit-y force-pushed the multi-storage branch from b17b36e to 345cd41 Compare June 28, 2018 21:23

Add test with single writer

71434f4

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

davit-y force-pushed the multi-storage branch from 05e4a23 to 71434f4 Compare June 28, 2018 22:29

jpkrohling mentioned this pull request Jun 29, 2018

Independent configurations for writing and reading #893

Closed

yurishkuro merged commit 07b2762 into jaegertracing:master Jun 29, 2018

yrsh mentioned this pull request Jul 24, 2018

Kafka configuration example #955

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Ability to use multiple storage types #880

Ability to use multiple storage types #880

davit-y commented Jun 22, 2018 •

edited

Loading

codecov bot commented Jun 22, 2018 •

edited

Loading

yurishkuro Jun 22, 2018

yurishkuro Jun 22, 2018

davit-y Jun 22, 2018

yurishkuro Jun 23, 2018

black-adder Jun 25, 2018

yurishkuro Jun 25, 2018

black-adder Jun 25, 2018

yurishkuro Jun 25, 2018

yurishkuro Jun 22, 2018

yurishkuro Jun 23, 2018

jpkrohling Jun 28, 2018

davit-y Jun 28, 2018

jpkrohling Jun 28, 2018

jpkrohling Jun 28, 2018

yurishkuro Jun 28, 2018

jpkrohling Jun 28, 2018

yurishkuro Jun 28, 2018

vprithvi Jun 28, 2018

yurishkuro Jun 28, 2018

yurishkuro Jun 28, 2018

yurishkuro Jun 28, 2018

yurishkuro Jun 28, 2018

yurishkuro Jun 28, 2018

Ability to use multiple storage types #880

Ability to use multiple storage types #880

Conversation

davit-y commented Jun 22, 2018 • edited Loading

Which problem is this PR solving?

Short description of the changes

codecov bot commented Jun 22, 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

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 commented Jun 22, 2018 •

edited

Loading

codecov bot commented Jun 22, 2018 •

edited

Loading