CLIENT-3365 Dynamic config changes #479
Conversation
mirzakaracic
force-pushed
the
CLIENT-3365_dynamic-config-changes
branch
from
02c9cbe to
6099646
Compare
mirzakaracic
changed the title
mirzakaracic
force-pushed
the
CLIENT-3365_dynamic-config-changes
branch
9 times, most recently
from
1fc1ff4 to
ceb6683
Compare
mirzakaracic
force-pushed
the
CLIENT-3365_dynamic-config-changes
branch
from
ceb6683 to
0c43acc
Compare
mirzakaracic
force-pushed
the
CLIENT-3365_dynamic-config-changes
branch
3 times, most recently
from
ffc8838 to
b4191a7
Compare
mirzakaracic
force-pushed
the
CLIENT-3365_dynamic-config-changes
branch
from
aac6597 to
f282446
Compare
mirzakaracic
force-pushed
the
CLIENT-3365_dynamic-config-changes
branch
6 times, most recently
from
67281a5 to
bbaf515
Compare
mirzakaracic
force-pushed
the
CLIENT-3365_dynamic-config-changes
branch
2 times, most recently
from
e8806c0 to
eea4478
Compare
mirzakaracic
force-pushed
the
CLIENT-3365_dynamic-config-changes
branch
from
eea4478 to
c9b5dc2
Compare
| @@ -58,7 +58,15 @@ func NewBatchDeletePolicy() *BatchDeletePolicy { | |||
| } | |||
| } | |||
|
|
|||
| func (bdp *BatchDeletePolicy) toWritePolicy(bp *BatchPolicy) *WritePolicy { | |||
| func NewBatchDeletePolicyOrDefaultFromCache(dynConfig *DynConfig) *BatchDeletePolicy { | |||
There was a problem hiding this comment.
I think a better name for it would be NewDynamicBatchDeletePolicy; Same for the rest of them.
The current name is not very Go like.
| return wp | ||
| } | ||
|
|
||
| // copyBatchDeletePolicy creates a new BasePolicy instance and copies the values from the source BatchDeletePolicy. | ||
| func copyBatchDeletePolicy(src *BatchDeletePolicy) *BatchDeletePolicy { |
There was a problem hiding this comment.
This should be a method:
func (bdp *BatchDeletePolicy) copy() *BatchDeletePolicy {
...
}Same for the rest.
| } | ||
|
|
||
| // applyConfigToBatchDeletePolicy applies the dynamic configuration and generates a new policy | ||
| func applyConfigToBatchDeletePolicy(policy *BatchDeletePolicy, dynConfig *DynConfig) *BatchDeletePolicy { |
There was a problem hiding this comment.
This also should be a method with a more idiomatic name:
func (bdp *BatchDeletePolicy) patchDynamic(dynConfig *DynConfig) *BatchDeletePolicy {
...
}| } | ||
| } | ||
|
|
||
| func mapDynamicBatchDeletePolicy(policy *BatchDeletePolicy, dynConfig *DynConfig) *BatchDeletePolicy { |
There was a problem hiding this comment.
Same here:
func (bdp *BatchDeletePolicy) mapDynamic(dynConfig *DynConfig) *BatchDeletePolicy {
...
}| cp := NewClientPolicy() | ||
|
|
||
| cfg := &dynconfig.Config{ | ||
| Static: &dynconfig.StaticConfig{ |
There was a problem hiding this comment.
What were the result of discussions regarding removing the static/dynamic sections for future compatibility?
| // ---------------------------------------------------------------- | ||
| // Main watch goroutine for the config provider | ||
| // ---------------------------------------------------------------- | ||
| func (dc *DynConfig) watchConfig(interval time.Duration) { |
There was a problem hiding this comment.
It's better for this interval to come from the config. Why does it not?
| // Enum types | ||
| // ---------------------------------------------------------------- | ||
|
|
||
| // TODO(Khosrow): Deal with the circular dependencies to remove the redefinition of these types. |
There was a problem hiding this comment.
Do the maps run just once when the config file is being read, or every time we patch user policies?
| ) | ||
|
|
||
| const ( | ||
| DSN_REGEX_PATTERN = `^\s*([A-Za-z][A-Za-z0-9+.-]*://)?(.*)$` |
There was a problem hiding this comment.
It's best to use named patterns so that you won't have to fiddle with the indices.
| responseTxnRollPolicy = copyTxnRollPolicy(policy) | ||
| responseTxnRollPolicy = mapDynamicTxnRollPolicy(responseTxnRollPolicy, dynConfig) | ||
|
|
||
| return responseTxnRollPolicy |
There was a problem hiding this comment.
By using methods, this would become a one liner:
return policy.copy().mapDynamic(dynConfig)
| IdleTimeout *int `yaml:"max_socket_idle"` | ||
| Timeout *int `yaml:"timeout"` | ||
| ErrorRateWindow *int `yaml:"error_rate_window"` | ||
| MaxErrorRate *int `yaml:"max_error_rate"` |
There was a problem hiding this comment.
This and a few more values are set in the ClientPolicy, but the clientPolicy attribute in the Cluster object is not turned in to an atomic.Pointer. That change needs to be done.
| Metrics *Metrics `yaml:"metrics"` | ||
| } | ||
|
|
||
| type Client struct { |
There was a problem hiding this comment.
Please check if all the yaml labels are the same for all clients.
| type Client struct { | ||
| // static config | ||
| ConfigInterval *int `yaml:"config_interval"` | ||
| ConnectionQueueSize *int `yaml:"connection_queue_size"` |
There was a problem hiding this comment.
Should be labeled as max_connections_per_node, same as Java. The labels should be unified.
Dynamic configuration changes for GO-lang client.