change cassandra timeout to duration var #1017
Conversation
replay
force-pushed
the
cassandra_timeout_format
branch
from
8ab79df to
7d3b44b
Compare
store/cassandra/config.go
Outdated
| @@ -67,7 +68,7 @@ func ConfigSetup() *flag.FlagSet { | |||
| cas.StringVar(&CliConfig.Keyspace, "keyspace", CliConfig.Keyspace, "cassandra keyspace to use for storing the metric data table") | |||
| cas.StringVar(&CliConfig.Consistency, "consistency", CliConfig.Consistency, "write consistency (any|one|two|three|quorum|all|local_quorum|each_quorum|local_one") | |||
| cas.StringVar(&CliConfig.HostSelectionPolicy, "host-selection-policy", CliConfig.HostSelectionPolicy, "") | |||
| cas.IntVar(&CliConfig.Timeout, "timeout", CliConfig.Timeout, "cassandra timeout in milliseconds") | |||
| cas.DurationVar(&CliConfig.Timeout, "timeout", CliConfig.Timeout, "cassandra timeout") | |||
There was a problem hiding this comment.
This will cause problems on existing MT deployments. The current config setting uses milliseconds, but when parsed as a duration they will now be treated as nanoseconds.
We need to ensure that doesnt happen
There was a problem hiding this comment.
Makes sense. Question is just how to ensure that this doesn't happen?
How about we add a second parameter that is a DurationVar, while leaving the old IntVar as it is, but if the IntVar is set in a config file or env we print a warning saying that's deprecated?
Both those parameters could be updating the same setting, with the new DurationVar having preference.
Then, in many months, we remove the old IntVar setting and only leave the new DurationVar.
There was a problem hiding this comment.
We could also make it simple and change this setting into a string var. Then, whenever the last character of the value is numeric, we parse it as int. If it is not numeric we parse it as duration.
There was a problem hiding this comment.
Lets combine those ideas.
- make it a stringVar
- if the value is just a number, treat it as ms, but log a warning that using a millisecond int is depreciated.
- otherwise parse it as a duration.
replay
force-pushed
the
cassandra_timeout_format
branch
2 times, most recently
from
155a223 to
92ddf6f
Compare
replay
force-pushed
the
cassandra_timeout_format
branch
from
92ddf6f to
ff60085
Compare
store/cassandra/cassandra.go
Outdated
| if err == nil { | ||
| return timeoutD | ||
| } | ||
| log.Warn("cassandra_store: invalid duration value %s, assuming default (1s)", timeout) |
There was a problem hiding this comment.
if the input can't be parsed as an int, nor as a duration specifier, we should error and abort.
just like we handle all other invalid configuration
replay
force-pushed
the
cassandra_timeout_format
branch
from
805c469 to
b84c0d7
Compare
|
@Dieterbe I updated the |
Fixes #818