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.

Sign up for GitHub

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

Jump to bottom

Move new etcd storage (low level storage) into cacher #30251

Merged
merged 1 commit into from
Aug 13, 2016
Merged

Move new etcd storage (low level storage) into cacher #30251

merged 1 commit into from
Aug 13, 2016

Conversation

hongchaodeng
Copy link
Contributor

@hongchaodeng hongchaodeng commented Aug 8, 2016 •
edited by k8s-oncall
Loading

In an effort for #29888, we are pushing forward this:

What?

  • It changes creating etcd storage.Interface impl into creating config
  • In creating cacher storage (StorageWithCacher), it passes config created above and new etcd storage inside.

Why?

  • We want to expose the information of (etcd) kv client to cacher. Cacher storage uses this information to talk to remote storage.

This change is Reviewable

@hongchaodeng
Copy link
Contributor Author

@xiang90

@k8s-github-robot k8s-github-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. release-note-label-needed labels Aug 8, 2016
@hongchaodeng hongchaodeng changed the title Move newing low level kv storage into inside cacher Move NewEtcdStorage (low level storage) into cacher Aug 9, 2016
@hongchaodeng hongchaodeng changed the title Move NewEtcdStorage (low level storage) into cacher Move new etcd storage (low level storage) into cacher Aug 9, 2016
@hongchaodeng hongchaodeng force-pushed the r2 branch 4 times, most recently from bc442b4 to cc63416 Compare August 9, 2016 16:55
@hongchaodeng
Copy link
Contributor Author

The unit test is a infra problem. I have make it pass in local.
I consider this ready for review.

@lavalamp @timothysc @xiang90

@hongchaodeng
Copy link
Contributor Author

@caesarxuchao Can you help review this?

@timothysc timothysc added area/etcd release-note-none Denotes a PR that doesn't merit a release note. sig/scalability Categorizes an issue or PR as relevant to SIG Scalability. and removed release-note-label-needed labels Aug 9, 2016
wojtek-t
wojtek-t reviewed Aug 10, 2016
View reviewed changes
pkg/registry/generic/options.go

// RESTOptions is set of configuration options to generic registries.
type RESTOptions struct {
Storage pkgstorage.Interface
Storage *storagebackend.Config
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 please rename Storage to StorageConfig (or sth that would actually mirror that it's not storage, but its config)?

@wojtek-t
Copy link
Member

Some naming comments, but mostly lgtm.

@hongchaodeng hongchaodeng force-pushed the r2 branch 3 times, most recently from deb2062 to 767e11a Compare August 10, 2016 22:55
@hongchaodeng
Copy link
Contributor Author

Test failure really is irrelevant:

ERROR: (gcloud.compute.firewall-rules.delete) Some requests did not succeed:
 - The resource 'projects/k8s-jkns-pr-gke/global/firewalls/e2e-gke-agent-pr-28-0-nodeports' was not found

@hongchaodeng
Copy link
Contributor Author

@k8s-bot test this issue: #IGNORE

@hongchaodeng
Copy link
Contributor Author

@wojtek-t
Addressed the comments. PTAL Thanks.
The test is mostly an infra problem. I will try to fix it and do squashing later.

@hongchaodeng
Copy link
Contributor Author

@wojtek-t
All tests passed. PTAL.

@hongchaodeng
Copy link
Contributor Author

@lavalamp
Can you apply 'lgtm' label on this? @wojtek-t only has some naming concern and it's all addressed.

@wojtek-t
Copy link
Member

Please squash commits - otherwise LGTM

@hongchaodeng
Copy link
Contributor Author

@wojtek-t
Squashed. Thanks!

@xiang90 xiang90 added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 12, 2016
@hongchaodeng
Copy link
Contributor Author

hongchaodeng commented Aug 12, 2016 •
edited
Loading

@k8s-bot unit test this issue: #IGNORE

Verified locally it's a flake:

FAIL    k8s.io/kubernetes/test/integration/garbagecollector

@k8s-github-robot k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 13, 2016
@k8s-github-robot k8s-github-robot removed lgtm "Looks good to me", indicates that a PR is ready to be merged. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Aug 13, 2016
@k8s-github-robot k8s-github-robot mentioned this pull request Aug 13, 2016
Closed
@k8s-bot
Copy link

k8s-bot commented Aug 13, 2016

GCE e2e build/test passed for commit d093809.

@xiang90 xiang90 added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 13, 2016
@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 Aug 13, 2016

GCE e2e build/test passed for commit d093809.

@k8s-github-robot
Copy link

Automatic merge from submit-queue

@k8s-github-robot k8s-github-robot merged commit e39d7f7 into kubernetes:master Aug 13, 2016
@hongchaodeng hongchaodeng deleted the r2 branch August 13, 2016 17:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@timothysc timothysc

@lavalamp lavalamp

@wojtek-t wojtek-t

Labels
area/etcd lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note-none Denotes a PR that doesn't merit a release note. sig/scalability Categorizes an issue or PR as relevant to SIG Scalability. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
9 participants
@hongchaodeng @wojtek-t @k8s-bot @k8s-github-robot @timothysc @lavalamp @xiang90 @googlebot @nikhiljindal