yaml: phase 1 #6070
yaml: phase 1 #6070
Conversation
Signed-off-by: Sugu Sougoumarane <[email protected]>
Signed-off-by: Sugu Sougoumarane <[email protected]>
Signed-off-by: Sugu Sougoumarane <[email protected]>
Signed-off-by: Sugu Sougoumarane <[email protected]>
Signed-off-by: Sugu Sougoumarane <[email protected]>
Signed-off-by: Sugu Sougoumarane <[email protected]>
Signed-off-by: Sugu Sougoumarane <[email protected]>
Signed-off-by: Sugu Sougoumarane <[email protected]>
Signed-off-by: Sugu Sougoumarane <[email protected]>
Signed-off-by: Sugu Sougoumarane <[email protected]>
Signed-off-by: Sugu Sougoumarane <[email protected]>
Signed-off-by: Sugu Sougoumarane <[email protected]>
Signed-off-by: Sugu Sougoumarane <[email protected]>
Signed-off-by: Sugu Sougoumarane <[email protected]>
There was a problem hiding this comment.
Have few questions
- Few flags are transformed to yaml config, rest would continue to be used as-is?
- If flag is provided and also it is present in yaml config. Is yaml config authoritative?
- Is there any new field added in yaml config which does not have a flag?
| dbaPool *dbconnpool.ConnectionPool | ||
| appDebugParams dbconfigs.Connector | ||
| } | ||
|
|
||
| // New creates a new Pool. The name is used | ||
| // to publish stats only. | ||
| func New(env tabletenv.Env, name string, capacity int, prefillParallelism int, timeout, idleTimeout time.Duration) *Pool { | ||
| func New(env tabletenv.Env, name string, cfg tabletenv.ConnPoolConfig) *Pool { |
There was a problem hiding this comment.
This is a go convention: we should avoid repeating the name of the package within contents of the package: https://blog.golang.org/package-names.
There was a problem hiding this comment.
Package name is connpool.
The struct name is Pool. This is still allowed as per the convention :)
| conns: connpool.New(env, "", 1, 0, 0, idleTimeout), | ||
| conns: connpool.New(env, "", tabletenv.ConnPoolConfig{ | ||
| Size: 1, | ||
| IdleTimeoutSeconds: env.Config().OltpReadPool.IdleTimeoutSeconds, |
There was a problem hiding this comment.
how is oltpReadPool vs olapReadPool config decided. Which one to pick?
There was a problem hiding this comment.
This is just backward compatible behavior. We need to make a decision.
| currentConfig.OlapReadPool.IdleTimeoutSeconds = currentConfig.OltpReadPool.IdleTimeoutSeconds | ||
| currentConfig.TxPool.IdleTimeoutSeconds = currentConfig.OltpReadPool.IdleTimeoutSeconds |
There was a problem hiding this comment.
will there be plan to use it separately.
There was a problem hiding this comment.
I don't know yet. This code just keeps it backward compatible. We should make a decision on it before making it official.
go.mod
Outdated
| @@ -157,11 +157,13 @@ require ( | |||
| gopkg.in/mgo.v2 v2.0.0-20160818020120-3f83fa500528 // indirect | |||
| gopkg.in/ory-am/dockertest.v3 v3.3.4 // indirect | |||
| gopkg.in/square/go-jose.v2 v2.3.1 // indirect | |||
| gopkg.in/yaml.v2 v2.2.8 | |||
There was a problem hiding this comment.
It got auto-added, but didn't get auto-removed.
Signed-off-by: Sugu Sougoumarane <[email protected]>
This is only a start. If we like this, we should look at transitioning all flags to yaml.
Yaml is authoritative. It will overwrite command line flags. We want to deprecate command line flags.
Not in this PR, but we'll soon be adding new elements that won't have a command line equivalents. |
| dbaPool *dbconnpool.ConnectionPool | ||
| appDebugParams dbconfigs.Connector | ||
| } | ||
|
|
||
| // New creates a new Pool. The name is used | ||
| // to publish stats only. | ||
| func New(env tabletenv.Env, name string, capacity int, prefillParallelism int, timeout, idleTimeout time.Duration) *Pool { | ||
| func New(env tabletenv.Env, name string, cfg tabletenv.ConnPoolConfig) *Pool { |
There was a problem hiding this comment.
Package name is connpool.
The struct name is Pool. This is still allowed as per the convention :)
Signed-off-by: Sugu Sougoumarane <[email protected]>
This implements part of #6068.
dbandinitstill need to be done. TabletConfig has been refactored to match the new specs, and a new flagtablet_confighas been added to vttablet. This flag is currently experimental and subject to change.