Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added flags for driving cassandra connection compression through config #1675

Merged
merged 4 commits into from
Jul 29, 2019
Merged

Added flags for driving cassandra connection compression through config #1675

merged 4 commits into from
Jul 29, 2019

Conversation

sagaranand015
Copy link
Contributor

Signed-off-by: Sagar Anand [email protected]

Which problem is this PR solving?

Short description of the changes

  • Added flag to drive the Cassandra Connection Compression from config. This will be use to use SnappyCompressor by default or opt out of using the SnappyCompressor

@codecov
Copy link

codecov bot commented Jul 22, 2019

Codecov Report

Merging #1675 into master will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1675      +/-   ##
==========================================
+ Coverage   98.49%   98.49%   +<.01%     
==========================================
  Files         193      193              
  Lines        9286     9291       +5     
==========================================
+ Hits         9146     9151       +5     
  Misses        111      111              
  Partials       29       29
Impacted Files Coverage Δ
plugin/storage/cassandra/options.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 f6fc248...54dd31a. Read the comment docs.

@sagaranand015
Copy link
Contributor Author

@yurishkuro : Could you review this PR please?
References: #1105 & #1667
Thanks

@@ -38,6 +38,7 @@ type Configuration struct {
MaxRetryAttempts int `validate:"min=0" yaml:"max_retry_attempt"`
ProtoVersion int `yaml:"proto_version"`
Consistency string `yaml:"consistency"`
EnableCompression bool `yaml:"enable-compression"`
Copy link
Member

Choose a reason for hiding this comment

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

This makes the struct not backwards compatible, because the default value will be false (only initializing from CLI flags would use true as default).

I prefer to call this DisableCompression

Copy link
Contributor Author

@sagaranand015 sagaranand015 Jul 24, 2019

Choose a reason for hiding this comment

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

@yurishkuro : Sure, disableCompression makes more sense, now that I think about it. Will make the changes.
Quick question though, the following puts the default value of enableCompression to be true. So, default as true also affects the backend compatibility?

	flagSet.Bool(
		nsConfig.namespace+suffixEnableCompression,
		true,
		"Enable the use of Compression while connecting to Cassandra API based Storage backend. Uses SnappyCompression by default")

Copy link
Member

Choose a reason for hiding this comment

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

This default only works if flags are used. But the CassandraConfiguration struct can also be used directly, e.g. it's how we do it in our customized binaries at Uber, where configuration is read from yaml files and auto-populated into config structs. So the struct itself should be backwards compatible.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for this @yurishkuro
Made the required changes, please review.

Copy link
Member

@yurishkuro yurishkuro left a comment

Choose a reason for hiding this comment

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

a couple of small tweaks

}

fmt.Println("Compression is:")
fmt.Println(cluster.Compressor)
Copy link
Member

Choose a reason for hiding this comment

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

please remove print statements, we don't do that in Jaeger. I believe at some point we simply log the whole config struct, using the logger, not println.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My bad..Removed these in the latest push

flagSet.Bool(
nsConfig.namespace+suffixDisableCompression,
false,
"Disables the use of the default Snappy Compression while connecting to the Cassandra Cluster if set to true")
Copy link
Member

Choose a reason for hiding this comment

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

may want to add to description: "This may be useful when using Azure Cosmos DB instead of Cassandra"

@sagaranand015
Copy link
Contributor Author

@yurishkuro : If everything seems ok, could you please merge this?
Thanks

@yurishkuro
Copy link
Member

All good, I was just waiting for a green build.

@yurishkuro yurishkuro merged commit 815ad51 into jaegertracing:master Jul 29, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Comsos DB as storage backend
2 participants