server: removed 'experimental' from warning-unary-request-duration flag #14414
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Codecov Report
@@ Coverage Diff @@
## main #14414 +/- ##
==========================================
- Coverage 75.66% 75.47% -0.19%
==========================================
Files 457 457
Lines 37320 37324 +4
==========================================
- Hits 28237 28172 -65
- Misses 7320 7369 +49
- Partials 1763 1783 +20
Flags with carried forward coverage won't be shown. Click here to find out more.
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
spzala
reviewed
Sep 1, 2022
There was a problem hiding this comment.
@lavacat I don't see any issues with graduating this feature from experimental but wondering if there was any discussion around the criteria/tests for moving it from experimental? Also, please sign-off your commit message to pass the DCO failure. Thanks!
lavacat
force-pushed
the
main-experimental
branch
from
90d4d9d to
64a9be8
Compare
|
The commit 2a26f7a , in which the flag was added, has never been cherry picked to But for this flag, it doesn't change the existing functionality, instead it just makes the value (previously hard code as 300ms) configurable, and defaults to the same previous default value (300ms). So I think the "experimental" should be removed in the first place. |
spzala
approved these changes
Sep 5, 2022
|
Based on the discussion in #14428, removing the |
|
@lavacat could you please resolve comment above so that we can merge this PR? |
|
any update on this? @lavacat |
ahrtr
requested changes
Oct 13, 2022
lavacat
force-pushed
the
main-experimental
branch
from
64a9be8 to
ffa171a
Compare
lavacat
force-pushed
the
main-experimental
branch
4 times, most recently
from
7f40dd2 to
947fefc
Compare
ahrtr
reviewed
Oct 28, 2022
| @@ -217,7 +217,7 @@ func StartEtcd(inCfg *Config) (e *Etcd, err error) { | |||
| WatchProgressNotifyInterval: cfg.ExperimentalWatchProgressNotifyInterval, | |||
| DowngradeCheckTime: cfg.ExperimentalDowngradeCheckTime, | |||
| WarningApplyDuration: cfg.ExperimentalWarningApplyDuration, | |||
| WarningUnaryRequestDuration: cfg.ExperimentalWarningUnaryRequestDuration, | |||
| WarningUnaryRequestDuration: cfg.WarningUnaryRequestDuration, | |||
There was a problem hiding this comment.
If users only set ExperimentalWarningUnaryRequestDuration, we should continue to respect it, but print a warning log. cc @spzala @serathius
There was a problem hiding this comment.
sounds good, I'll add warning log about flag being depreciated. FYI, '--experimental ...' version of the flag is broken at the moment. I'm going to push another update
lavacat
force-pushed
the
main-experimental
branch
2 times, most recently
from
3e05dee to
abee748
Compare
|
@ahrtr ready for review. Used your suggestions, added warning and duplicate test for experimental flag |
ahrtr
reviewed
Nov 12, 2022
|
It still has a flaw: if both flags are specified by users, then the experimental one is used. This isn't correct. The rough logic should be something like below, |
|
Please also add a changelog item. |
lavacat
force-pushed
the
main-experimental
branch
2 times, most recently
from
46c0c93 to
1b947b6
Compare
ahrtr
reviewed
Nov 15, 2022
server/embed/etcd.go
Outdated
Comment on lines
165
to
168
| warningUnaryRequestDuration, err := cfg.parseWarningUnaryRequestDuration() | ||
| if err != nil { | ||
| return e, err | ||
| } |
There was a problem hiding this comment.
The comment #14414 (comment) is still valid.
There was a problem hiding this comment.
@ahrtr I was following this grouping of parsing functions https://github.com/etcd-io/etcd/blob/main/server/embed/etcd.go#L158
parseCompactionRetention and parseBackendFreelistType.
But since this change is about flags (not values), I've moved into config.parse per your suggestion.
There was a problem hiding this comment.
Points from my side:
- The new method
parseWarningUnaryRequestDurationlooks good to me; - I don't think it's good to add too much configuration parsing code inside function
StartEtcd, and the example you followed might need to be moved into(cfg *config)parse as well. But it's unrelated to this PR, so feel free to resolve it in a separate PR.
lavacat
force-pushed
the
main-experimental
branch
2 times, most recently
from
2b0e114 to
c4f6dd1
Compare
ahrtr
reviewed
Nov 15, 2022
CHANGELOG/CHANGELOG-3.6.md
Outdated
| @@ -19,6 +19,7 @@ See [code changes](https://github.com/etcd-io/etcd/compare/v3.5.0...v3.6.0). | |||
|
|
|||
| - Deprecated [V2 discovery](https://etcd.io/docs/v3.5/dev-internal/discovery_protocol/). | |||
| - Deprecated [SetKeepAlive and SetKeepAlivePeriod in limitListenerConn](https://github.com/etcd-io/etcd/pull/14356). | |||
| - Removed [`experimental` from `--experimental-warning-unary-request-duration`](https://github.com/etcd-io/etcd/pull/14414) | |||
There was a problem hiding this comment.
Suggested change
| - Removed [`experimental` from `--experimental-warning-unary-request-duration`](https://github.com/etcd-io/etcd/pull/14414) | |
| - Promoted [`--experimental-warning-unary-request-duration` to `--warning-unary-request-duration`](https://github.com/etcd-io/etcd/pull/14414). Note the experimental flag has already been deprecated and will be decommissioned in v3.7. |
lavacat
force-pushed
the
main-experimental
branch
from
c4f6dd1 to
024e839
Compare
ahrtr
reviewed
Nov 16, 2022
| @@ -19,7 +19,7 @@ See [code changes](https://github.com/etcd-io/etcd/compare/v3.5.0...v3.6.0). | |||
|
|
|||
| - Deprecated [V2 discovery](https://etcd.io/docs/v3.5/dev-internal/discovery_protocol/). | |||
| - Deprecated [SetKeepAlive and SetKeepAlivePeriod in limitListenerConn](https://github.com/etcd-io/etcd/pull/14356). | |||
| - Removed [etcdctl defrag --data-dir](https://github.com/etcd-io/etcd/pull/13793). | |||
| - Promoted [`--experimental-warning-unary-request-duration` to `--warning-unary-request-duration`](https://github.com/etcd-io/etcd/pull/14414). Note the experimental has already been deprecated and will be decommissioned in v3.7.- Removed [etcdctl defrag --data-dir](https://github.com/etcd-io/etcd/pull/13793). | |||
There was a problem hiding this comment.
Suggested change
| - Promoted [`--experimental-warning-unary-request-duration` to `--warning-unary-request-duration`](https://github.com/etcd-io/etcd/pull/14414). Note the experimental has already been deprecated and will be decommissioned in v3.7.- Removed [etcdctl defrag --data-dir](https://github.com/etcd-io/etcd/pull/13793). | |
| - Promoted [`--experimental-warning-unary-request-duration` to `--warning-unary-request-duration`](https://github.com/etcd-io/etcd/pull/14414). Note the experimental flag has already been deprecated and will be decommissioned in v3.7. | |
| - Removed [etcdctl defrag --data-dir](https://github.com/etcd-io/etcd/pull/13793). |
|
Please rebase this PR as well to resolve the conflict. |
lavacat
force-pushed
the
main-experimental
branch
from
024e839 to
5c5525e
Compare
|
Failing e2e test is TestCtlV3Lock. Possibly related to #14763 |
--warning-unary-request-duration is a duplicate of --experimental-warning-unary-request-duration experimental-warning-unary-request-duration will be removed in v3.7. fixes etcd-io#13783 Signed-off-by: Bogdan Kanivets <bkanivets@apple.com>
lavacat
force-pushed
the
main-experimental
branch
from
5c5525e to
663581b
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
fixes #13783
Please read https://github.com/etcd-io/etcd/blob/main/CONTRIBUTING.md#contribution-flow.