support go time package format gc time #996
Conversation
|
No release note, Please follow https://github.com/pingcap/community/blob/master/contributors/release-note-checker.md |
|
|
|
@july2993 Already support both int and string and add some tests. PTAL again. |
pump/config.go
Outdated
| @@ -176,6 +177,15 @@ func (cfg *Config) Parse(arguments []string) error { | |||
| util.AdjustString(&cfg.DataDir, defaultDataDir) | |||
| util.AdjustInt(&cfg.HeartbeatInterval, defaultHeartbeatInterval) | |||
|
|
|||
| if cfg.GC.Duration == 0 && cfg.GCStr == "" { | |||
There was a problem hiding this comment.
this park looks confusing, when we both set int the config file and the command, the command line value should take effect instead of the one in the config file.
can we change
type Duration string
``
and using the same object for both the config file and command line flag?
There was a problem hiding this comment.
Actually this part can make it to prioritize the config in the command. If we set type Duration to string, how do we get the actual duration? Unmarshall it every time?
There was a problem hiding this comment.
Can you explain it more clearly?
There was a problem hiding this comment.
Actually this part can make it to prioritize the config in the command
yes, I got it wrong.
If we set type Duration to string, how do we get the actual duration? Unmarshall it every time?
I think parsing it every time is ok here.
I am ok with this now, this will not block this pr.
pump/config.go
Outdated
| @@ -100,7 +101,7 @@ func NewConfig() *Config { | |||
| fs.StringVar(&cfg.EtcdURLs, "pd-urls", defaultEtcdURLs, "a comma separated list of the PD endpoints") | |||
| fs.StringVar(&cfg.DataDir, "data-dir", "", "the path to store binlog data") | |||
| fs.IntVar(&cfg.HeartbeatInterval, "heartbeat-interval", defaultHeartbeatInterval, "number of seconds between heartbeat ticks") | |||
| fs.IntVar(&cfg.GC, "gc", defaultGC, "recycle binlog files older than gc days") | |||
| fs.StringVar(&cfg.GCStr, "gc", "", "recycle binlog files older than gc time. default unit is day. also accept 8h format time(max unit is hour)") | |||
There was a problem hiding this comment.
set default value, so user can see the default value like before?
There was a problem hiding this comment.
If we set default value here, the value in toml will always be covered by default "7". Can we add it in the prompt?
There was a problem hiding this comment.
ok, that's using two objects making it complex。
| *d = Duration(s) | ||
| return nil | ||
| } | ||
|
|
There was a problem hiding this comment.
var _ toml.** = &Duration{}
--
add this make sure it impl the interface and let's reader know this.
|
/run-all-tests |
1 similar comment
|
/run-all-tests |
|
/run-all-tests |
|
cherry pick to release-3.1 in PR #997 |
ti-srebot
added
already-cherry-pick-3.1
and removed
needs-cherry-pick-release-3.1
labels
|
cherry pick to release-3.0 failed |
What problem does this PR solve?
resolve #994
What is changed and how it works?
Support gc time for "8m", "7h". If no unit is specified, gc duration will use day as unit as before.
It should be noticed that the max time duration unit that supported in golang is hour "h".
golang/go#11473
golang/go#17767
Check List
Tests
Code changes
Related changes
Release note