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

Refactor code for creating Cacher. #16584

Merged
merged 1 commit into from
Nov 3, 2015

Conversation

wojtek-t
Copy link
Member

Requested in #16496

@k8s-github-robot
Copy link

Labelling this PR as size/M

@k8s-github-robot k8s-github-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Oct 30, 2015
@k8s-bot
Copy link

k8s-bot commented Oct 30, 2015

GCE e2e test build/test passed for commit 24101ce27033d7a007ea09cbf87eac87c904296e.

@wojtek-t
Copy link
Member Author

@k8s-bot unit test this please

1 similar comment
@wojtek-t
Copy link
Member Author

@k8s-bot unit test this please

@wojtek-t
Copy link
Member Author

@lavalamp - PTAL

NewListFunc: func() runtime.Object { return &api.PodList{} },
}
storageInterface = storage.NewCacher(config)
storageInterface = storage.NewCacher(1000, s, nil, &api.Pod{}, prefix, true, newListFunc)
Copy link
Member

Choose a reason for hiding this comment

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

I was actually hoping to construct the cacher outside and pass it in-- if that's too hard, then I'll take this, though.

Copy link
Member Author

Choose a reason for hiding this comment

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

It's different for every resource, so yeah - that would be difficult.

Copy link
Member

Choose a reason for hiding this comment

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

(Discussed in person: I suggested passing a construction function in instead of the boolean)

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

@wojtek-t
Copy link
Member Author

wojtek-t commented Nov 2, 2015

@lavalamp - PTAL

[Any better suggestions for names are appreciated]

@k8s-github-robot
Copy link

Labelling this PR as size/L

@k8s-github-robot 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 Nov 2, 2015
@@ -111,10 +111,56 @@ type Cacher struct {
ListFromCache bool
}

type DecorateStorage func(
Interface, int, Versioner, runtime.Object, string, bool,
func() runtime.Object) Interface
Copy link
Member

Choose a reason for hiding this comment

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

Can you name the parameters? + add godoc.

I might call this a storage factory...

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

@wojtek-t
Copy link
Member Author

wojtek-t commented Nov 2, 2015

@lavalamp - PTAL

@k8s-bot
Copy link

k8s-bot commented Nov 2, 2015

GCE e2e test build/test passed for commit ed0abe5bdf87cdccafcea8d19416ee06c9b824a2.

@k8s-bot
Copy link

k8s-bot commented Nov 2, 2015

GCE e2e test build/test passed for commit 8f385c5.

@lavalamp lavalamp added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 2, 2015
@lavalamp
Copy link
Member

lavalamp commented Nov 2, 2015

LGTM, thanks for bearing with me :)

@k8s-github-robot
Copy link

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

@k8s-bot
Copy link

k8s-bot commented Nov 3, 2015

GCE e2e test build/test passed for commit 8f385c5.

@k8s-github-robot
Copy link

Automatic merge from submit-queue

k8s-github-robot pushed a commit that referenced this pull request Nov 3, 2015
@k8s-github-robot k8s-github-robot merged commit b793795 into kubernetes:master Nov 3, 2015
@wojtek-t wojtek-t deleted the refactor_use_cacher branch November 5, 2015 08:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lgtm "Looks good to me", indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants