Skip to content
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

clientv3: version checking #7342

Merged
merged 3 commits into from
Feb 21, 2017
Merged

Conversation

heyitsanthony
Copy link
Contributor

Adds a UseLatestCluster flag (and user-defined context) to config.

Fixes #6579

@gyuho
Copy link
Contributor

gyuho commented Feb 16, 2017

So this only checks against clients <3.2 trying to connect to 3.2> with this flag on, right?

func (c *Client) checkVersion() (err error) {
var wg sync.WaitGroup
errc := make(chan error, len(c.cfg.Endpoints))
ctx, cancel := context.WithCancel(c.ctx)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does cancel func preserver even when we overwrite ctx?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, canceling the parent context will cancel any descendents

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh I see it now context.WithTimeout(ctx, not context.WithTimeout(c.ctx.

Makes sense.

@@ -41,6 +42,13 @@ type Config struct {
// Password is a password for authentication.
Password string `json:"password"`

// UseLatestCluster when set will refuse to create a client against an outdated cluster.
UseLatestCluster bool `json:"use-latest-cluster"`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

EnsureClusterVersion? Or CheckClusterVersion? use is not a strong enough word I feel.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

RejectOldCluster? Should have something about whether it's new/old/latest

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

RejectOldCluster looks good.

min, rerr = strconv.Atoi(vs[1])
}
if maj < 3 || min < 2 {
rerr = rpctypes.ErrNotCapable
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

shall we define a different error for cluster version mismatch?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the fence about it. I feel it's similar enough to NotCapable (i.e., a "soft" not capable) that the error can be reused.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NotCapable is the error returned from server (at least for its current definition). The string is "etcdserver: not capable". It has the server prefix. But here users would expect client: server is not capable.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, fair enough

@heyitsanthony heyitsanthony force-pushed the client-version branch 2 times, most recently from 9ebccd8 to fba1925 Compare February 17, 2017 02:12
@codecov-io
Copy link

Codecov Report

❗ No coverage uploaded for pull request base (master@a5cf7fd). Click here to learn what that means.
The diff coverage is 78.37%.

@@            Coverage Diff            @@
##             master    #7342   +/-   ##
=========================================
  Coverage          ?   63.67%           
=========================================
  Files             ?      233           
  Lines             ?    21016           
  Branches          ?        0           
=========================================
  Hits              ?    13381           
  Misses            ?     6627           
  Partials          ?     1008
Impacted Files Coverage Δ
clientv3/client.go 61.47% <78.37%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a5cf7fd...51435df. Read the comment docs.

@heyitsanthony
Copy link
Contributor Author

all fixed ptal

@gyuho
Copy link
Contributor

gyuho commented Feb 21, 2017

LGTM. /cc @xiang90

@xiang90
Copy link
Contributor

xiang90 commented Feb 21, 2017

LGTM

@heyitsanthony heyitsanthony merged commit 0c0fbbd into etcd-io:master Feb 21, 2017
@heyitsanthony heyitsanthony deleted the client-version branch February 21, 2017 19:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants