clientv3/integration: Don't retry other endpoints when err == rpctypes.ErrAuthNotEnable by cfc4n · Pull Request #10428 · etcd-io/etcd

cfc4n · 2019-01-24T16:52:22Z

codecov-io · 2019-01-24T18:19:42Z

Codecov Report

Merging #10428 into master will decrease coverage by 0.05%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master   #10428      +/-   ##
==========================================
- Coverage   71.61%   71.56%   -0.06%     
==========================================
  Files         392      392              
  Lines       36489    36491       +2     
==========================================
- Hits        26131    26113      -18     
- Misses       8531     8560      +29     
+ Partials     1827     1818       -9

Impacted Files	Coverage Δ
clientv3/client.go	`78.99% <100%> (+0.11%)`	⬆️
proxy/grpcproxy/register.go	`69.44% <0%> (-11.12%)`	⬇️
pkg/adt/interval_tree.go	`78.67% <0%> (-7.81%)`	⬇️
clientv3/leasing/util.go	`91.66% <0%> (-6.67%)`	⬇️
clientv3/namespace/watch.go	`87.87% <0%> (-6.07%)`	⬇️
proxy/grpcproxy/watch.go	`88.55% <0%> (-4.22%)`	⬇️
etcdserver/v2_server.go	`80.76% <0%> (-3.85%)`	⬇️
pkg/testutil/recorder.go	`77.77% <0%> (-3.71%)`	⬇️
raft/progress.go	`94.17% <0%> (-1.95%)`	⬇️
proxy/grpcproxy/watch_broadcasts.go	`92.53% <0%> (-1.5%)`	⬇️
... and 17 more

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 3e0f0ba...a033686. Read the comment docs.

xiang90 · 2019-01-24T18:52:38Z

@cfc4n can you add a test?

cfc4n · 2019-01-25T02:52:08Z

@cfc4n can you add a test?

OK，please wait...

cfc4n · 2019-01-25T11:47:52Z

@xiang90 Is my test code correct?

spzala

@cfc4n some thoughts while we wait to hear from @xiang90 Thanks!

spzala · 2019-01-25T13:29:36Z

 		resp, err = auth.authenticate(ctx, c.Username, c.Password)
 		if err != nil {
+			if err == rpctypes.ErrAuthNotEnabled {
+				// return err if err == rpctypes.ErrAuthNotEnable ,Don't retry other endpoints


I would suggest to just say something like "// return err without retrying other endpoints" (to keep it clean as the if condition is self explanatory)

Sure,Thanks @spzala

No problem @cfc4n and thank you for addressing the suggestions!

spzala · 2019-01-25T13:30:45Z

 }
+
+
+// fixes # https://github.com/etcd-io/etcd/issues/10430


Should remove this comment.

spzala · 2019-01-25T13:31:09Z

+// fixes # https://github.com/etcd-io/etcd/issues/10430
+func ExampleGetTokenWithoutAuth() {
+	var tmpEndpoints = []string{}
+	fmt.Println("it will append 100 times into {tmpEndpoints} with {endpoints}.")


Also, I don't think this print statement is needed.

mitake · 2019-01-25T16:59:43Z

@xiang90 got it, I'll take a look probably this weekend.
@cfc4n the idea seems to be good, thanks! But could you fix the commit and coding style? The problems can be detected by executing PASSES=fmt ./test on your repository.

cfc4n · 2019-01-26T10:08:54Z

@mitake Done,I'll do it next time.thanks.

xiang90 · 2019-01-29T19:00:07Z

@cfc4n

Can you add the test as an integration test here: https://github.com/etcd-io/etcd/tree/master/clientv3/integration instead of an example? thanks!

mitake · 2019-01-30T00:22:28Z

@cfc4n could you fix errors from fmt test and build errors reported in https://travis-ci.com/etcd-io/etcd/jobs/173656226 ?

xiang90 · 2019-01-31T20:32:36Z

+	}
+
+	client, err = clientv3.New(cfg)
+


defer client.Close()

xiang90 · 2019-01-31T20:32:45Z

+	case nil:
+		t.Log("passes as expected, but may be connection time less than DialTimeout")
+	case context.DeadlineExceeded:
+		t.Fatalf("not expected result:%v with endpoint:%s", err, authapi.Endpoints())


xiang90 · 2019-01-31T20:32:50Z

+	case rpctypes.ErrAuthNotEnabled:
+		t.Logf("passes as expected:%v", err)
+	default:
+		t.Fatalf("other errors:%v", err)


xiang90 · 2019-01-31T20:33:22Z

please squash commits into one with the right prefix "clientv3". thanks!

cfc4n · 2019-02-01T06:23:17Z

please squash commits into one with the right prefix "clientv3". thanks!

@xiang90 Is it like commit 9e9c7b9 ?

xiang90 · 2019-02-01T07:37:26Z

@cfc4n

Now you have 10 commits in one issue. You need to follow this:

https://github.com/todotxt/todo.txt-android/wiki/Squash-All-Commits-Related-to-a-Single-Issue-into-a-Single-Commit

cfc4n · 2019-02-01T07:57:16Z

Got it ,thanks.

xiang90 · 2019-02-01T18:33:54Z

@cfc4n

hmm... there are now 12 commits in this PR.

Can you try

git reset --soft HEAD~12 # reset all commits but keep the changes
git commit -am "clientv3/integration: return err if err == rpctypes.ErrAuthNotEnable" # make a commit
git push -f xxx # force push to update the PR

xiang90 · 2019-02-02T04:10:58Z

+	default:
+		t.Errorf("other errors:%v", err)
+	}
+	defer client.Close()


well. if you still put client.Close() here, then no need to add defer. move this to line 138, right after the creation of the client instead.

cfc4n · 2019-02-02T07:52:14Z

@xiang90 Is it correct now? But I'm also not understand the report in https://travis-ci.com/etcd-io/etcd/jobs/174849460 ,why it's failed at Job 666.4\ Job 666.9\ Job 666.11 and, what can I do for this report? It seems nothing to do with my PR.

xiang90 · 2019-02-02T21:37:51Z

the test failure has nothing to do with this RP. LGTM. Thanks for the contribution!

jingyih · 2019-02-11T19:16:29Z

cc @jpbetz

xiang90 assigned xiang90 and mitake Jan 24, 2019

spzala reviewed Jan 25, 2019

View reviewed changes

xiang90 reviewed Jan 31, 2019

View reviewed changes

Comment thread clientv3/integration/user_test.go

}

client, err = clientv3.New(cfg)

Copy link
Copy Markdown

Contributor

xiang90 Jan 31, 2019

Choose a reason for hiding this comment

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

defer client.Close()

xiang90 reviewed Jan 31, 2019

View reviewed changes

cfc4n closed this Feb 2, 2019

cfc4n reopened this Feb 2, 2019

cfc4n closed this Feb 2, 2019

cfc4n reopened this Feb 2, 2019

cfc4n changed the title ~~Don't retry other endpoints when err == rpctypes.ErrAuthNotEnable~~ clientv3/integration: Don't retry other endpoints when err == rpctypes.ErrAuthNotEnable Feb 2, 2019

xiang90 reviewed Feb 2, 2019

View reviewed changes

cfc4n closed this Feb 2, 2019

clientv3/integration: return err if err == rpctypes.ErrAuthNotEnable

a033686

cfc4n reopened this Feb 2, 2019

xiang90 merged commit be40b1d into etcd-io:master Feb 2, 2019

devster31 mentioned this pull request Apr 13, 2019

[etcdctl] Error: context deadline exceeded #10642

Closed

cfc4n mentioned this pull request Aug 2, 2020

clientv3:get AuthToken gracefully without extra connection. #12165

Merged

Conversation

cfc4n commented Jan 24, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov-io commented Jan 24, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

xiang90 commented Jan 24, 2019

Uh oh!

cfc4n commented Jan 25, 2019

Uh oh!

cfc4n commented Jan 25, 2019

Uh oh!

spzala left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mitake commented Jan 25, 2019

Uh oh!

cfc4n commented Jan 26, 2019

Uh oh!

xiang90 commented Jan 29, 2019

Uh oh!

mitake commented Jan 30, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xiang90 commented Jan 31, 2019

Uh oh!

cfc4n commented Feb 1, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

xiang90 commented Feb 1, 2019

Uh oh!

cfc4n commented Feb 1, 2019

Uh oh!

xiang90 commented Feb 1, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cfc4n commented Feb 2, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

xiang90 commented Feb 2, 2019

Uh oh!

jingyih commented Feb 11, 2019

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

6 participants

cfc4n commented Jan 24, 2019 •

edited

Loading

codecov-io commented Jan 24, 2019 •

edited

Loading

cfc4n commented Feb 1, 2019 •

edited

Loading

cfc4n commented Feb 2, 2019 •

edited

Loading