server: Added config parameter experimental-apply-warning-duration #12448

agargi · 2020-11-04T03:05:00Z

server: Added config parameter apply-warning-duration

jingyih

Thanks! My preference would be to introduce this new flag as an experimental flag in etcd v3.5. This is usually the case when a new flag is being added.

agargi · 2020-11-08T14:01:47Z

Thanks! My preference would be to introduce this new flag as an experimental flag in etcd v3.5. This is usually the case when a new flag is being added.

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?

jingyih

I added suggestion on making the flag description more precise. Otherwise LGTM. Thanks!

jingyih · 2020-11-09T15:21:22Z

server/etcdmain/config.go

@@ -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.")


"Time duration after which a warning is generated if applying request takes more time."

+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

@spzala @jingyih Changes recommended done & modified code committed

jingyih · 2020-11-09T15:21:48Z

server/embed/config.go

@@ -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


*if applying request takes more time than this value.

spzala

lgtm but few nits. Thanks @agargi

spzala · 2020-11-09T15:45:38Z

server/etcdmain/config.go

@@ -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.")


+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

spzala · 2020-11-09T15:45:43Z

server/etcdmain/help.go

@@ -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.


+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.?

jingyih · 2020-11-16T17:17:32Z

server/embed/config.go

@@ -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"`


NIT: could you move this flag to the code block around line 290? It looks like all the experimental flags are grouped there.

@jingyih Done

jingyih · 2020-11-16T17:24:46Z

server/embed/config.go

@@ -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


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

@jingyih Done

spzala · 2020-11-18T02:57:47Z

lgtm @agargi thanks!

Manual cherry pick of #12448 on release 3.4

agargi mentioned this pull request Nov 4, 2020

"took too long to execute" always in etcd logs #10860

Closed

jingyih reviewed Nov 4, 2020

View reviewed changes

jingyih mentioned this pull request Nov 5, 2020

read-only range request took too long to execute #11912

Closed

agargi force-pushed the introduce_config_parameter branch from fc0d3a7 to 7d3ddf2 Compare November 8, 2020 13:31

agargi changed the title ~~server: Added config parameter apply-warning-duration~~ server: Added config parameter experimental-apply-warning-duration Nov 8, 2020

jingyih reviewed Nov 9, 2020

View reviewed changes

spzala reviewed Nov 9, 2020

View reviewed changes

agargi force-pushed the introduce_config_parameter branch 2 times, most recently from 09d7bd9 to b00069f Compare November 14, 2020 21:35

jingyih reviewed Nov 16, 2020

View reviewed changes

server: Added config parameter experimental-warning-apply-duration

c1c681a

agargi force-pushed the introduce_config_parameter branch from b00069f to c1c681a Compare November 17, 2020 22:34

jingyih merged commit c11ddc6 into etcd-io:master Nov 18, 2020

trisomeyr mentioned this pull request Feb 16, 2021

v3.4.14; etcd flag provided but not defined: -experimental-warning-apply-duration #12693

Closed

hexfusion mentioned this pull request Mar 3, 2021

Manual cherry pick of #12448 on release 3.4 #12740

Merged

hexfusion added a commit that referenced this pull request Mar 3, 2021

Merge pull request #12740 from hexfusion/cp-12448--release-3.4

afd6d8a

Manual cherry pick of #12448 on release 3.4

hexfusion mentioned this pull request Jun 18, 2021

ETCD-199: bump etcd v3.4.16 openshift/etcd#83

Merged

lavacat mentioned this pull request Mar 23, 2022

Track stabilization of --experimental-warning-apply-duration flag #13780

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

server: Added config parameter experimental-apply-warning-duration #12448

server: Added config parameter experimental-apply-warning-duration #12448

agargi commented Nov 4, 2020 •

edited

Loading

jingyih left a comment

agargi commented Nov 8, 2020

jingyih left a comment

jingyih Nov 9, 2020

spzala Nov 9, 2020

agargi Nov 15, 2020

jingyih Nov 9, 2020

spzala left a comment

spzala Nov 9, 2020

spzala Nov 9, 2020

jingyih Nov 16, 2020

agargi Nov 18, 2020

jingyih Nov 16, 2020

agargi Nov 18, 2020

spzala commented Nov 18, 2020

server: Added config parameter experimental-apply-warning-duration #12448

server: Added config parameter experimental-apply-warning-duration #12448

Conversation

agargi commented Nov 4, 2020 • edited Loading

jingyih left a comment

Choose a reason for hiding this comment

agargi commented Nov 8, 2020

jingyih left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

spzala left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

spzala commented Nov 18, 2020

agargi commented Nov 4, 2020 •

edited

Loading