server: Added config parameter experimental-apply-warning-duration #12448
Conversation
agargi
force-pushed
the
introduce_config_parameter
branch
from
fc0d3a7
to
7d3ddf2
Compare
Converted this into an experimental flag and pushed changes. Build is however failing but failure does not seem to be related to the change. Is there a way I can re-trigger the build? |
agargi
changed the title
server/etcdmain/config.go
Outdated
| @@ -254,6 +254,7 @@ func newConfig() *config { | |||
| fs.IntVar(&cfg.ec.ExperimentalCompactionBatchLimit, "experimental-compaction-batch-limit", cfg.ec.ExperimentalCompactionBatchLimit, "Sets the maximum revisions deleted in each compaction batch.") | |||
| fs.DurationVar(&cfg.ec.ExperimentalWatchProgressNotifyInterval, "experimental-watch-progress-notify-interval", cfg.ec.ExperimentalWatchProgressNotifyInterval, "Duration of periodic watch progress notifications.") | |||
| fs.DurationVar(&cfg.ec.ExperimentalDowngradeCheckTime, "experimental-downgrade-check-time", cfg.ec.ExperimentalDowngradeCheckTime, "Duration of time between two downgrade status check.") | |||
| fs.DurationVar(&cfg.ec.WarnApplyDuration, "experimental-apply-warning-duration", cfg.ec.WarnApplyDuration, "Time duration after which a warning is generated if request takes more time.") | |||
There was a problem hiding this comment.
"Time duration after which a warning is generated if applying request takes more time."
There was a problem hiding this comment.
+1 @jingyih Also, couple nits, we should keep the naming consistent i.e. 1) Use Experimental prefix for the parameter name similar to other experimental ones. e.g. ExperimentalDowngradeCheckTime 2) Considering the name is not long, good idea to use WarningApplyDuration 3) Keeping consistent with naming, suggest to use experimental-warning-apply-duration instead of experimental-apply-warning-duration
server/embed/config.go
Outdated
| @@ -178,6 +179,10 @@ type Config struct { | |||
| MaxTxnOps uint `json:"max-txn-ops"` | |||
| MaxRequestBytes uint `json:"max-request-bytes"` | |||
|
|
|||
| // WarnApplyDuration is time duration after which a warning is generated if request | |||
There was a problem hiding this comment.
*if applying request takes more time than this value.
server/etcdmain/config.go
Outdated
| @@ -254,6 +254,7 @@ func newConfig() *config { | |||
| fs.IntVar(&cfg.ec.ExperimentalCompactionBatchLimit, "experimental-compaction-batch-limit", cfg.ec.ExperimentalCompactionBatchLimit, "Sets the maximum revisions deleted in each compaction batch.") | |||
| fs.DurationVar(&cfg.ec.ExperimentalWatchProgressNotifyInterval, "experimental-watch-progress-notify-interval", cfg.ec.ExperimentalWatchProgressNotifyInterval, "Duration of periodic watch progress notifications.") | |||
| fs.DurationVar(&cfg.ec.ExperimentalDowngradeCheckTime, "experimental-downgrade-check-time", cfg.ec.ExperimentalDowngradeCheckTime, "Duration of time between two downgrade status check.") | |||
| fs.DurationVar(&cfg.ec.WarnApplyDuration, "experimental-apply-warning-duration", cfg.ec.WarnApplyDuration, "Time duration after which a warning is generated if request takes more time.") | |||
There was a problem hiding this comment.
+1 @jingyih Also, couple nits, we should keep the naming consistent i.e. 1) Use Experimental prefix for the parameter name similar to other experimental ones. e.g. ExperimentalDowngradeCheckTime 2) Considering the name is not long, good idea to use WarningApplyDuration 3) Keeping consistent with naming, suggest to use experimental-warning-apply-duration instead of experimental-apply-warning-duration
server/etcdmain/help.go
Outdated
| @@ -214,6 +214,8 @@ Experimental feature: | |||
| Skip verification of SAN field in client certificate for peer connections. | |||
| --experimental-watch-progress-notify-interval '10m' | |||
| Duration of periodical watch progress notification. | |||
| --experimental-apply-warning-duration '100ms' | |||
| Warning is generated if certain requests take more than this duration. | |||
There was a problem hiding this comment.
+1 @jingyih Also, certain requests is confusing here. I guess we can simply say Warning is generated if requests take more time than this duration.?
agargi
force-pushed
the
introduce_config_parameter
branch
2 times, most recently
from
09d7bd9
to
b00069f
Compare
server/embed/config.go
Outdated
| @@ -178,6 +179,10 @@ type Config struct { | |||
| MaxTxnOps uint `json:"max-txn-ops"` | |||
| MaxRequestBytes uint `json:"max-request-bytes"` | |||
|
|
|||
| // This is the time duration after which a warning is generated if applying request | |||
| // takes more time than this value. | |||
| ExperimentalWarningApplyDuration time.Duration `json:"experimental-warning-apply-duration"` | |||
There was a problem hiding this comment.
NIT: could you move this flag to the code block around line 290? It looks like all the experimental flags are grouped there.
server/embed/config.go
Outdated
| @@ -178,6 +179,10 @@ type Config struct { | |||
| MaxTxnOps uint `json:"max-txn-ops"` | |||
| MaxRequestBytes uint `json:"max-request-bytes"` | |||
|
|
|||
| // This is the time duration after which a warning is generated if applying request | |||
There was a problem hiding this comment.
By convention, the comment should start with the flag name, i.e., s/This/ExperimentalWarningApplyDuration. Otherwise the goword checking will complain: https://travis-ci.com/github/etcd-io/etcd/jobs/440054426#L442
agargi
force-pushed
the
introduce_config_parameter
branch
from
b00069f
to
c1c681a
Compare
|
lgtm @agargi thanks! |
Manual cherry pick of #12448 on release 3.4
server: Added config parameter apply-warning-duration
Refer: #10860