-
Notifications
You must be signed in to change notification settings - Fork 9.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[Etcd Downgrade] Store downgrade info to backend #11725
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good in general. Thanks!
etcdserver/api/membership/store.go
Outdated
tx.UnsafePut(clusterBucketName, ckey, []byte(ver.String())) | ||
tx.Unlock() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a reason we changed line 75? In case UnsafePut fails, the deferred statement seems better.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think there is only one line between Lock()
and Unlock()
so that it is not necessary to use defer()
? If UnsafePut fails, why using defer is better?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I meant if UnsafePut panics, defer ensures that the tx is unlocked.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Understood. Changed to defer statement.
Codecov Report
@@ Coverage Diff @@
## master #11725 +/- ##
==========================================
+ Coverage 66.49% 66.68% +0.19%
==========================================
Files 403 403
Lines 36881 36935 +54
==========================================
+ Hits 24524 24631 +107
+ Misses 10855 10816 -39
+ Partials 1502 1488 -14
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lgtm
etcdserver/api/membership/cluster.go
Outdated
type DowngradeInfo struct { | ||
// TargetVersion is the target downgrade version, if the cluster is not under downgrading, | ||
// the targetVersion will be an empty string | ||
TargetVersion string |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should we tag these fields rather than encoding them in camel case? e.g. json:"target-version"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm, thx!
Let's update CHANGELOG in a separate PR.
Thanks! I will update the CHANGELOG after finishing the server side of downgrade. |
@@ -691,6 +702,39 @@ func clusterVersionFromBackend(lg *zap.Logger, be backend.Backend) *semver.Versi | |||
return semver.Must(semver.NewVersion(string(vals[0]))) | |||
} | |||
|
|||
func downgradeInfoFromBackend(lg *zap.Logger, be backend.Backend) *DowngradeInfo { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is an unexported func without caller.
Can it be removed ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The caller of this func will be added in following PRs. I want to put the getter and setter of downgradeInfo in one PR.
Issue #11716
As a field of cluster, downgrade info is stored in backend.
This PR added the getter and setter of it.
@gyuho @jingyih