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

etcd3/store: support TTL in Create, Update #23995

Merged
merged 1 commit into from
Apr 20, 2016
Merged

etcd3/store: support TTL in Create, Update #23995

merged 1 commit into from
Apr 20, 2016

Conversation

hongchaodeng
Copy link
Contributor

@hongchaodeng hongchaodeng commented Apr 7, 2016 •
edited
Loading

ref: #22448

What's included in the PR?

  • Support TTL keys in Create() and Update()
  • Simple testing for create

Note

  • Have TTL keys in a long window (5min) in one lease. I'm not sure if we should do it right now. We could just implement it as simple as is. Once we have etcd3 work done we could start testing and measuring with some realistic load.

@hongchaodeng hongchaodeng mentioned this pull request Apr 7, 2016
Closed
@k8s-github-robot k8s-github-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. release-note-label-needed labels Apr 7, 2016
xiang90
xiang90 reviewed Apr 7, 2016
View reviewed changes
pkg/storage/etcd3/store.go
@@ -118,10 +118,16 @@ func (s *store) Create(ctx context.Context, key string, obj, out runtime.Object,
}
key = keyWithPrefix(s.pathPrefix, key)

leaseOpt, err := s.createLeaseOpt(ctx, int64(ttl))
Copy link
Contributor

Choose a reason for hiding this comment

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

if ttl != 0

Copy link
Contributor

Choose a reason for hiding this comment

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

nvm. you put this into the opt

Copy link
Contributor

Choose a reason for hiding this comment

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

we should rename this to s.createOptForTTL to hide the lease concept entirely.

@k8s-bot
Copy link

k8s-bot commented Apr 7, 2016

GCE e2e build/test passed for commit 6f79f2c0575ef3c2b2019811a237f398dc8e4ab4.

@hongchaodeng
Copy link
Contributor Author

Raising Xiang's comment to the top level:

one lease per ttl key is expensive. i would suggest to great one lease for every 10 minutes window. at lease, we should add a todo.

This seems fine to me. My thought is that we need update docs in interface that says "ttl is an approximate number".

@xiang90
Copy link
Contributor

xiang90 commented Apr 7, 2016

This seems fine to me. My thought is that we need update docs in interface that says "ttl is an approximate number".

I believe so. Based on the offline discussion with @lavalamp, it seems that TTL is only used for GC for now. So we are free to modify the comment on the interface I guess.

@xiang90
Copy link
Contributor

xiang90 commented Apr 7, 2016

@hongchaodeng As long as we do not create 100s of leases per second, it should be fine.

@k8s-bot
Copy link

k8s-bot commented Apr 7, 2016

GCE e2e build/test passed for commit 44847012ee017c42015b617fa5d51c2db1337b69.

lavalamp
lavalamp reviewed Apr 11, 2016
View reviewed changes
pkg/registry/generic/etcd/etcd.go
@@ -330,10 +330,7 @@ func (e *Etcd) Update(ctx api.Context, obj runtime.Object) (runtime.Object, bool
if err != nil {
return nil, nil, err
}
if int64(ttl) != res.TTL {
Copy link
Member

Choose a reason for hiding this comment

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

I suggest not making this change?

@lavalamp
Copy link
Member

We might create 100's of events per second under heavy load (big cluster, many pods starting at once). I'm not sure what the sustained rate would be-- probably much less than that?

@lavalamp lavalamp assigned wojtek-t and unassigned lavalamp Apr 11, 2016
@k8s-github-robot k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 17, 2016
@wojtek-t
Copy link
Member

@hongchaodeng - please rebase

@k8s-github-robot k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 17, 2016
@k8s-bot
Copy link

k8s-bot commented Apr 17, 2016

GCE e2e build/test passed for commit 6465ce042bc3c9b0eb543fb3f37ec3ab4ec4abba.

@wojtek-t
Copy link
Member

Thanks - will take a look tomorrow. Sorry for delay.

@hongchaodeng hongchaodeng mentioned this pull request Apr 17, 2016
Merged
wojtek-t
wojtek-t reviewed Apr 18, 2016
View reviewed changes
pkg/storage/etcd3/store.go

// createOpts creates the etcd client options based on given parameters.
// ttl: if ttl is non-zero, it will attach the key to a lease with ttl of roughly equivalent length
func (s *store) createOpts(ctx context.Context, ttl int64) ([]clientv3.OpOption, error) {
Copy link
Member

Choose a reason for hiding this comment

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

This actually is just about TTL, not about create. Can you please name it like "ttlOpts" (or "ttlCreateOpts") or sth like that?

@wojtek-t
Copy link
Member

Added some comments. But I will wait for the watcher PR to be merge before lgtm-ing this one.

@wojtek-t wojtek-t added release-note-none Denotes a PR that doesn't merit a release note. and removed release-note-label-needed labels Apr 18, 2016
@xiang90
Copy link
Contributor

xiang90 commented Apr 18, 2016

@wojtek-t Watch pr is merged :)

@hongchaodeng
Copy link
Contributor Author

Addressed. PTAL.

@k8s-bot
Copy link

k8s-bot commented Apr 19, 2016

GCE e2e build/test passed for commit 3e9405822eb9c84108607f4cdd6dcd291d36b233.

@k8s-bot
Copy link

k8s-bot commented Apr 19, 2016

GCE e2e build/test passed for commit 46214c6.

@wojtek-t wojtek-t added lgtm "Looks good to me", indicates that a PR is ready to be merged. ok-to-merge and removed needs-ok-to-merge labels Apr 19, 2016
@wojtek-t
Copy link
Member

LGTM

@k8s-bot
Copy link

k8s-bot commented Apr 20, 2016

GCE e2e build/test passed for commit 46214c6.

@k8s-bot
Copy link

k8s-bot commented Apr 20, 2016

GCE e2e build/test passed for commit 46214c6.

@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 Apr 20, 2016

GCE e2e build/test passed for commit 46214c6.

@k8s-github-robot
Copy link

Automatic merge from submit-queue

@k8s-github-robot k8s-github-robot merged commit 7f362b1 into kubernetes:master Apr 20, 2016
@hongchaodeng hongchaodeng deleted the ttl branch April 20, 2016 21:36
@hongchaodeng hongchaodeng mentioned this pull request Jul 22, 2016
Closed
30 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@wojtek-t wojtek-t

Labels
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. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
8 participants
@hongchaodeng @k8s-bot @xiang90 @lavalamp @wojtek-t @k8s-github-robot @k8s-teamcity-mesosphere @googlebot