Allow configuration of Cassandra's host selection policy #4069
Conversation
ChinYing-Li
changed the title
| @@ -63,6 +64,7 @@ func (cfg *Config) RegisterFlags(f *flag.FlagSet) { | |||
| f.BoolVar(&cfg.DisableInitialHostLookup, "cassandra.disable-initial-host-lookup", false, "Instruct the cassandra driver to not attempt to get host info from the system.peers table.") | |||
| f.BoolVar(&cfg.SSL, "cassandra.ssl", false, "Use SSL when connecting to cassandra instances.") | |||
| f.BoolVar(&cfg.HostVerification, "cassandra.host-verification", true, "Require SSL certificate validation.") | |||
| f.StringVar(&cfg.HostSelectionPolicy, "cassandra.host-selection-policy", "", "Policy for selecting Cassandra host.") | |||
There was a problem hiding this comment.
To improve the documentation for this, perhaps we could set the default (instead of checking for ""), and list the supported options.
| f.StringVar(&cfg.HostSelectionPolicy, "cassandra.host-selection-policy", "", "Policy for selecting Cassandra host.") | |
| f.StringVar(&cfg.HostSelectionPolicy, "cassandra.host-selection-policy", "RoundRobin", "Policy for selecting Cassandra host. Supported values are: RoundRobin, TokenAware.") |
| @@ -180,6 +182,15 @@ func (cfg *Config) setClusterConfig(cluster *gocql.ClusterConfig) error { | |||
| } | |||
| } | |||
| } | |||
|
|
|||
| if cfg.HostSelectionPolicy == "RoundRobin" { | |||
There was a problem hiding this comment.
suggestion(non-blocking): consider defining RoundRobin and TokenAware as constant enums. For example:
const (
SelectionPolicyRoundRobin = "RoundRobin"
SelectionPolicyTokenAware = "TokenAware"
)| @@ -63,6 +64,7 @@ func (cfg *Config) RegisterFlags(f *flag.FlagSet) { | |||
| f.BoolVar(&cfg.DisableInitialHostLookup, "cassandra.disable-initial-host-lookup", false, "Instruct the cassandra driver to not attempt to get host info from the system.peers table.") | |||
| f.BoolVar(&cfg.SSL, "cassandra.ssl", false, "Use SSL when connecting to cassandra instances.") | |||
| f.BoolVar(&cfg.HostVerification, "cassandra.host-verification", true, "Require SSL certificate validation.") | |||
| f.StringVar(&cfg.HostSelectionPolicy, "cassandra.host-selection-policy", "RoundRobin", "Policy for selecting Cassandra host. Supported values are: RoundRobin, TokenAware.") | |||
There was a problem hiding this comment.
nit: I'm not crazy about the config values requiring capitalization. Would it be possible to rename the config values to be round-robin and token-aware? Or is there an existing convention that would make the current values preferable?
There was a problem hiding this comment.
I chose the config values simply because I didn't find similar StringVar for reference. We can avoid capitalization.
…ct#4069) Signed-off-by: ChinYing-Li <[email protected]>
There was a problem hiding this comment.
LGTM! Good work.
@ChinYing-Li Please rebase onto master when you get a chance.
|
@ChinYing-Li it looks like you merged master into your branch instead of rebasing. This makes the PR much less readable and didn't fix the conflict with master. Would you mind cleaning this up to just be the commits you added on top of the master branch? |
We do this quite often, and it's also what Github does when you use "Resolve conflicts" button in the UI. Github shows changed files correctly (as does Goland, haven't checked other tools). Why do you write that it makes PR much less readable? Note that we always squash when merging to master, so in the end, it looks like single commit on the master anyway (svn-style :)) |
Signed-off-by: ChinYing-Li [email protected]
What this PR does:
Though I am not an expert of Cassandra (learning!), I took reference from uber/peloton.
The default
HostSelectionPolicyisRoundRobinHostPolicy; onlyRoundRobinHostPolicyandTokenAwareSelectionPolicyare supported.Any suggestion is appreciated!
Which issue(s) this PR fixes:
Fixes #2561
Checklist
CHANGELOG.mdupdated - the order of entries should be[CHANGE],[FEATURE],[ENHANCEMENT],[BUGFIX]