Add Cassandra table options flag #2575

kwSeo · 2020-05-10T12:02:05Z

What this PR does:
This PR adds cassandra.table-options flag and storage.cassandra.table_options configuration option to customize Cassandra table options when creating index or chunk table by table-manager.

Configuration Example

storage:
  engine: "chunks"
  cassandra:
    addresses: "127.0.0.1"
    port: 9042
    keyspace: "cortex"
    consistency: "ONE"
    auth: false
    table_options: "CLUSTERING ORDER BY (range DESC)
      AND comment = 'Important records'
      AND compaction = { 'class' : 'LeveledCompactionStrategy' }"

EDIT: rename table_with -> table_options

Which issue(s) this PR fixes:
Fixes #2453

Checklist

Tests updated
Documentation added
CHANGELOG.md updated - the order of entries should be [CHANGE], [FEATURE], [ENHANCEMENT], [BUGFIX]

jtlisi

Overall I like the idea of being able to configure this. However, I think it would be better to do it using a well defined struct instead of just passing a string.

jtlisi · 2020-05-10T19:47:46Z

pkg/chunk/cassandra/table_client.go

+			PRIMARY KEY (hash, range)
+		)`, desc.Name)
+	if c.cfg.TableWith != "" {
+		query = fmt.Sprintf("%s WITH %s", query, c.cfg.TableWith)


IMO, joining a string like this for a query to Cassandra is dangerous. If the end goal is to change the compaction strategy, I think that would be better handled with a CompactionConfig struct that a parameterizes all of the possible compactions strategies into a config and generates the appropriate query string.

@jtlisi @pracucci

Thank you for review! I agree your opinion.
And there are some considerations.
Some of table options for Cassandra are related to duration, like this:

gc_grace_seconds = 864000 default_time_to_live = 0 tombstone_compaction_interval = 864000 max_sstable_age_days = 100 ...

I thought table options like above are suitable for time.Duration type.
But some of names for table options indicate time unit.
I am not sure if time.Duration is suitable.
There is a way to rename the table options but I'm concerned about user confusion.
I am curious about your opinion.

Since this is specifically for Cassandra,I recommend using types that will make input correspond to Cassandra. That way users already familiar with the system will be able to directly copy config values over.

So in the cases you mentioned above I would use int values. Just make sure you document each option in the flag help text.

@jtlisi

In our use case, we are using ScyllaDB, which is compatible but contains many options that are not default in Cassandra.

https://docs.scylladb.com/getting-started/ddl/
CTRL-F scylla_encryption_options

It is a bad idea to assume what flags the Cassandra server will support. In my opinion this should be made extensible and the current PR does that nicely.

This is a niche flag that will only be used by those who fully understand the implications of changing the flag, better solved by documentation, and already have authorization on the Cassandra server through the configuration.

@jtlisi
In addition to @coelho, there may be another use cases, such as DataStax products.
Also, table options depend on the Cassandra version.
It is hard to find all options and consider all differences.
My goals have been just caching and compaction strategy.
But this issue seems to have more considerations than I think.
If you still think joining string is dangerous, it may need another way.

I see your point, and I agree in some cases a separate string makes sense. @pracucci WDYT?

jtlisi · 2020-05-19T19:42:45Z

pkg/chunk/cassandra/storage_client.go

@@ -66,6 +67,7 @@ func (cfg *Config) RegisterFlags(f *flag.FlagSet) {
 	f.DurationVar(&cfg.MinBackoff, "cassandra.retry-min-backoff", 100*time.Millisecond, "Minimum time to wait before retrying a failed request. (Default = 100ms)")
 	f.DurationVar(&cfg.MaxBackoff, "cassandra.retry-max-backoff", 10*time.Second, "Maximum time to wait before retrying a failed request. (Default = 10s)")
 	f.IntVar(&cfg.QueryConcurrency, "cassandra.query-concurrency", 0, "Limit number of concurrent queries to Cassandra. (Default is 0: no limit)")
+	f.StringVar(&cfg.TableWith, "cassandra.table-with", "", "Table options used to create index or chunk tables.(Default = \"\": use default table options of Cassandra")


I would recommend naming this variable TableOptions and the flag table-options.

Also the help text should explain that the provided string will be concatenated to all table creation operations.

@jtlisi
I have renamed table-with to table-options and improved the help text.

jtlisi · 2020-05-19T19:46:17Z

docs/configuration/config-file-reference.md

+  # Table options used to create index or chunk tables.(Default = "": use
+  # default table options of Cassandra
+  # CLI flag: -cassandra.table-with
+  [table_with: <string> | default = ""]


Be sure to update https://github.com/cortexproject/cortex/blob/master/docs/production/storage-cassandra.md with information about this flag. As well as warnings about the potential issues a user could face by misconfiguring it.

@jtlisi @coelho
I have written the description about table_options to the storage-cassandra.md.
I think I am not that good at English...
Please check if there is anything strange about the contents.

LGTM
EDIT: Great English btw, no worries :)

jtlisi

LGTM

Please rebase when you get a chance.

kwSeo · 2020-06-03T02:13:48Z

@jtlisi
Thank you for your review.
I have done rebase.

Signed-off-by: Kyeongwon Seo <[email protected]>

…unning Cortex with Cassandra Signed-off-by: Kyeongwon Seo <[email protected]>

Signed-off-by: Kyeongwon Seo <[email protected]>

pracucci

LGTM, thanks!

pull-request-size bot added the size/M label May 10, 2020

jtlisi suggested changes May 10, 2020

View reviewed changes

jtlisi suggested changes May 19, 2020

View reviewed changes

pull-request-size bot added size/L and removed size/M labels May 23, 2020

kwSeo requested a review from jtlisi May 31, 2020 15:14

jtlisi approved these changes Jun 2, 2020

View reviewed changes

kwSeo force-pushed the add_cassandra_flags branch from 2474ab0 to 3caa5bb Compare June 3, 2020 01:04

kwSeo requested a review from jtlisi June 4, 2020 13:44

pull-request-size bot added size/XXL and removed size/L labels Jun 8, 2020

kwSeo force-pushed the add_cassandra_flags branch from b3180a1 to 3caa5bb Compare June 8, 2020 15:09

pull-request-size bot added size/L and removed size/XXL labels Jun 8, 2020

kwSeo added 8 commits June 9, 2020 00:10

add the cassandra.table-with flag

da18e99

Signed-off-by: Kyeongwon Seo <[email protected]>

update CHANGELOG and docs

6be3193

Signed-off-by: Kyeongwon Seo <[email protected]>

add PR number to CHANGELOG

a1625d7

Signed-off-by: Kyeongwon Seo <[email protected]>

remove blacklisted package import

71384ce

Signed-off-by: Kyeongwon Seo <[email protected]>

fix import with goimports

c27eb78

Signed-off-by: Kyeongwon Seo <[email protected]>

rename table_with to table_options and improve the flag usage

5e7668f

Signed-off-by: Kyeongwon Seo <[email protected]>

add a description about configuring table options to documentation: R…

f78dfe9

…unning Cortex with Cassandra Signed-off-by: Kyeongwon Seo <[email protected]>

clean white noise

8a55066

Signed-off-by: Kyeongwon Seo <[email protected]>

kwSeo force-pushed the add_cassandra_flags branch from 3caa5bb to 8a55066 Compare June 8, 2020 15:13

pracucci approved these changes Jun 8, 2020

View reviewed changes

jtlisi merged commit 5f0a3e8 into cortexproject:master Jun 8, 2020

kwSeo deleted the add_cassandra_flags branch June 10, 2020 06:40

This was referenced Jun 24, 2020

Fix invalid change log that exist in the wrong section #2781

Closed

Fix invalid change log that exist in the wrong section (based on release-1.2 branch) #2787

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Cassandra table options flag #2575

Add Cassandra table options flag #2575

kwSeo commented May 10, 2020 •

edited

Loading

jtlisi left a comment

jtlisi May 10, 2020

kwSeo May 14, 2020

jtlisi May 14, 2020

coelho May 16, 2020 •

edited

Loading

kwSeo May 17, 2020

jtlisi May 19, 2020

jtlisi May 19, 2020

kwSeo May 23, 2020

jtlisi May 19, 2020

kwSeo May 23, 2020 •

edited

Loading

coelho May 23, 2020 •

edited

Loading

jtlisi left a comment •

edited

Loading

kwSeo commented Jun 3, 2020

pracucci left a comment

Add Cassandra table options flag #2575

Add Cassandra table options flag #2575

Conversation

kwSeo commented May 10, 2020 • edited Loading

jtlisi left a 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

coelho May 16, 2020 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kwSeo May 23, 2020 • edited Loading

Choose a reason for hiding this comment

coelho May 23, 2020 • edited Loading

Choose a reason for hiding this comment

jtlisi left a comment • edited Loading

Choose a reason for hiding this comment

kwSeo commented Jun 3, 2020

pracucci left a comment

Choose a reason for hiding this comment

kwSeo commented May 10, 2020 •

edited

Loading

coelho May 16, 2020 •

edited

Loading

kwSeo May 23, 2020 •

edited

Loading

coelho May 23, 2020 •

edited

Loading

jtlisi left a comment •

edited

Loading