Refactor storePodsNamespacer.List() by mml · Pull Request #25895 · kubernetes/kubernetes

mml · 2016-05-19T18:28:03Z

This fixes a bug in the previous version where, when we fell back on a
brute force approach, we were still returning an error.

It also clarifies the flow control into 3 distinct cases. The cases
don't share variables any more, which makes mistakes like the one
mentioned above harder.

lavalamp · 2016-05-19T19:47:41Z

code change looks like a big improvement. Should I be asking for a new test?

mml · 2016-05-19T19:49:07Z

@lavalamp what do you have in mind?

lavalamp · 2016-05-19T19:59:02Z

@mml maybe of the brute force list block, since I think it probably wasn't getting used/exercised at all before?

mml · 2016-05-19T22:27:59Z

@lavalamp test added, and I verified that it failed on upstream/master. PTAL

mml · 2016-05-20T00:03:41Z

Huh.

Running tests for APIVersion: v1,extensions/v1beta1 with etcdPrefix: kubernetes.io/registry
+++ [0519 16:06:04] Running tests without code coverage
[...]
FAIL    k8s.io/kubernetes/test/integration  406.207s

mml · 2016-05-20T00:28:13Z

@k8s-bot retest this please #25539

mml · 2016-05-20T00:44:32Z

@k8s-bot test this issue: #25539

mml · 2016-05-20T04:23:57Z

Well, this is a new one.

--- FAIL: TestReflectorResync (0.01s)
    reflector_test.go:380: exactly 2 iterations were expected, got: 3
[...]
FAIL
FAIL    k8s.io/kubernetes/pkg/client/cache  0.884s

Passes on my workstation. I'm running it in a loop overnight hoping for a repeat failure.

mml · 2016-05-20T15:08:00Z

I ran this 1000 times last night with 0 failures.

@k8s-bot unit test this issue: #25964

0xmichalis · 2016-05-22T15:01:58Z

storeReplicationControllersNamespacer seems to have the same problem

mml · 2016-05-23T17:21:56Z

@Kargakis I see that now. There's also inconsistency. storePodsNamespacer returns a PodList, but storeReplicationControllerNamespacer returns []ReplicationController. Glancing at the rest of the file, I can't tell which is the idiom we want to use. (A type-parameterized way to define this interface would be nice. I guess we could do that with Yet More Codegen.)

@lavalamp please hold off on review. I'll go ahead and test/fix the ReplicationController implementation in the same way.

mml · 2016-05-23T18:43:46Z

@lavalamp OK PTAL. I recommend looking at each commit separately. I will squash once the review is done.

In the end, I realized upon looking at the way storeReplicationControllerNamespacer.List was implemented that the simple control flow works pretty easily as long as you never touch the return values until you get into one of the three cases. Both functions are exactly the same now.

I think I uncovered yet another bug, though, which I'll file a new issue about.

mml · 2016-05-23T23:05:47Z

Updated again to fix #26107

0xmichalis · 2016-05-24T10:35:59Z

pkg/client/cache/listers.go

I am not sure this is optimal. At the very least, you should add a comment explaining why this needs to happen. Unfortunately named return values don't help much either.

lavalamp · 2016-05-24T21:51:40Z

pkg/client/cache/listers.go

return pods, err

I don't care myself, but some people find it less confusing.

I'll make the change.

Would be nice to have a clear style guide and enforce it with a verify-* check.

lavalamp · 2016-05-24T21:52:11Z

pkg/client/cache/listers.go

same for all

lavalamp · 2016-05-24T21:55:28Z

lgtm-- squash & add label yourself.

Refactor storePodsNamespacer.List() and storeReplicationContollersNamespacer.List(). They are the same function, just with different signatures. This fixes a bug where, when we fell back on a brute force approach, we were still returning an error. Also change to explicit return without named return values.

k8s-github-robot · 2016-05-26T03:44:35Z

@k8s-bot test this [submit-queue is verifying that this PR is safe to merge]

k8s-bot · 2016-05-26T04:21:38Z

GCE e2e build/test passed for commit 1fee311.

k8s-github-robot · 2016-05-26T04:50:52Z

Automatic merge from submit-queue

Automatic merge from submit-queue Implement DisruptionController. Part of #12611 This currently also includes a pending commit from #25895

mml assigned lavalamp May 19, 2016

mml added this to the v1.3 milestone May 19, 2016

mml added priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. release-note-none Denotes a PR that doesn't merit a release note. labels May 19, 2016

k8s-github-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label May 19, 2016

googlebot added the cla: yes label May 19, 2016

mml force-pushed the storePodNamespacer.List branch from 5271dbf to 074953f Compare May 19, 2016 22:23

k8s-github-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels May 19, 2016

mml mentioned this pull request May 19, 2016

Implement DisruptionController. #25921

Merged

mml mentioned this pull request May 20, 2016

pkg/client/cache flake: reflector_test.go:380: exactly 2 iterations were expected, got: 3 #25964

Closed

mml mentioned this pull request May 20, 2016

Unit test flake: TestReflectorResync exactly 2 iterations were expected, got: 3 #25953

Closed

mml mentioned this pull request May 23, 2016

TestStoreToReplicationControllerLister does not reset state between cases #26107

Closed

mml force-pushed the storePodNamespacer.List branch from ca2fcd6 to bd5dcc8 Compare May 23, 2016 23:05

0xmichalis reviewed May 24, 2016
View reviewed changes

mml force-pushed the storePodNamespacer.List branch from 17cbb34 to 9a9c061 Compare May 24, 2016 21:11

lavalamp reviewed May 24, 2016
View reviewed changes

pkg/client/cache/listers.go Outdated

Copy link
Copy Markdown

Contributor

lavalamp May 24, 2016

Choose a reason for hiding this comment

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

same for all

mml force-pushed the storePodNamespacer.List branch from 9a9c061 to 1fee311 Compare May 26, 2016 00:19

mml added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 26, 2016

alex-mohr mentioned this pull request May 26, 2016

Submit queue's unit tests taking more than 1 hour to run #26337

Closed

k8s-github-robot merged commit 34a640d into kubernetes:master May 26, 2016

k8s-github-robot pushed a commit that referenced this pull request Aug 17, 2016

Merge pull request #25921 from mml/db.controller

d574082

Automatic merge from submit-queue Implement DisruptionController. Part of #12611 This currently also includes a pending commit from #25895

Conversation

mml commented May 19, 2016

Uh oh!

lavalamp commented May 19, 2016

Uh oh!

mml commented May 19, 2016

Uh oh!

lavalamp commented May 19, 2016

Uh oh!

mml commented May 19, 2016

Uh oh!

mml commented May 20, 2016

Uh oh!

mml commented May 20, 2016

Uh oh!

mml commented May 20, 2016

Uh oh!

mml commented May 20, 2016

Uh oh!

mml commented May 20, 2016

Uh oh!

0xmichalis commented May 22, 2016

Uh oh!

mml commented May 23, 2016

Uh oh!

mml commented May 23, 2016

Uh oh!

mml commented May 23, 2016

Uh oh!

0xmichalis May 24, 2016

Choose a reason for hiding this comment

Uh oh!

mml May 24, 2016

Choose a reason for hiding this comment

Uh oh!

lavalamp May 24, 2016

Choose a reason for hiding this comment

Uh oh!

mml May 24, 2016

Choose a reason for hiding this comment

Uh oh!

lavalamp May 24, 2016

Choose a reason for hiding this comment

Uh oh!

lavalamp commented May 24, 2016

Uh oh!

k8s-github-robot commented May 26, 2016

Uh oh!

k8s-bot commented May 26, 2016

Uh oh!

k8s-github-robot commented May 26, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants