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

etcd3/store: watcher implementation #23694

Merged
merged 1 commit into from
Apr 18, 2016
Merged

Conversation

hongchaodeng
Copy link
Contributor

@hongchaodeng hongchaodeng commented Mar 31, 2016

ref: #22448

This PR does:

  • Provide a watcher that uses etcd v3 API to watch changes via etcd and process them based on existing logic of storage.Interface.Watch(), WatchList().
  • By using the watcher, very trivial to implement Watch() and WatchList() in etcd3 storage.Interface implementation.

go func() {
for _, kv := range getResp.Kvs {
wc.sendEvent(&event{
key: string(kv.Key),
Copy link
Contributor

Choose a reason for hiding this comment

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

can we use parseEvent for this too?

@wojtek-t wojtek-t self-assigned this Mar 31, 2016
@wojtek-t
Copy link
Member

Let me take a look into it later - I have a full calendar while being in US, but hopefully will take a first pass on it later today (or in the worst case, tomorrow).

@k8s-github-robot k8s-github-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Mar 31, 2016
@k8s-bot
Copy link

k8s-bot commented Mar 31, 2016

GCE e2e build/test passed for commit 3e9950c0071a32683dfa7ba9193573a54b7787bf.

@timothysc
Copy link
Member

Will look tomorrow.

@k8s-bot
Copy link

k8s-bot commented Mar 31, 2016

GCE e2e build/test passed for commit c58a63e28d91ef2d896d8fc04d3345b258edf7d4.

@xiang90
Copy link
Contributor

xiang90 commented Mar 31, 2016

/cc @kubernetes/huawei

@k8s-bot
Copy link

k8s-bot commented Apr 1, 2016

GCE e2e build/test passed for commit 729bd2fc34770b023e9a8a674a4167e24215f53a.

@hongchaodeng
Copy link
Contributor Author

Test failed because of data race. See etcd-io/etcd#4876

I will have to bump up godep again.

@k8s-bot
Copy link

k8s-bot commented Apr 1, 2016

GCE e2e build/test failed for commit 04882ca2e576c0c19f9d39d14752751a453b1bfe.

Please reference the list of currently known flakes when examining this failure. If you request a re-test, you must reference the issue describing the flake.

@k8s-github-robot k8s-github-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Apr 2, 2016
@k8s-bot
Copy link

k8s-bot commented Apr 2, 2016

GCE e2e build/test passed for commit dadab986b46aa516c69a1257e5dc4927c60e3af5.

@k8s-bot
Copy link

k8s-bot commented Apr 2, 2016

GCE e2e build/test passed for commit fafda38e3c0e6b0978efa283deae394dc6fad9f4.

@k8s-bot
Copy link

k8s-bot commented Apr 2, 2016

GCE e2e build/test failed for commit 61fe10bcfe0fd89c9dcae93ddfa0c41568d2b44e.

Please reference the list of currently known flakes when examining this failure. If you request a re-test, you must reference the issue describing the flake.

@k8s-bot
Copy link

k8s-bot commented Apr 2, 2016

GCE e2e build/test passed for commit acf2db98f008c83af71714b343525f97b5dadda0.

@k8s-bot
Copy link

k8s-bot commented Apr 3, 2016

GCE e2e build/test passed for commit c421d9632816dbbc999a80fc9c80662d9495736c.

key: string(etcdEvent.Kv.Key),
value: etcdEvent.Kv.Value,
rev: etcdEvent.Kv.ModRevision,
isDeleted: etcdEvent.Type == storagepb.DELETE || etcdEvent.Type == storagepb.EXPIRE,
Copy link
Member

Choose a reason for hiding this comment

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

I would almost think this abstraction could be in the client library.. same with the one below.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can you clarify?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, I see what you mean.

I'm considering adding it to etcd library too. I will try to coordinate with the etcd team on this.

@k8s-bot
Copy link

k8s-bot commented Apr 17, 2016

GCE e2e build/test passed for commit a69c15e0cf5fd7565a6648ce57ff07f09e897436.

k8s-github-robot pushed a commit that referenced this pull request Apr 17, 2016
Automatic merge from submit-queue

Bump up etcd dependency to fix data race

ref: #23694

What this PR does
- Bumping up the godep of etcd to fix data race in etcd watcher. Without this change, watcher PR builds will fail in race detection.
- Small changes to fix builds after upgrade
@hongchaodeng hongchaodeng changed the title etcd v3 watcher implementation etcd3/store: watcher implementation Apr 17, 2016
@hongchaodeng
Copy link
Contributor Author

Rebased.
We need to get this in before the TTL PR #23995 and then update there with watcher.
@wojtek-t @timothysc PTAL

@k8s-bot
Copy link

k8s-bot commented Apr 17, 2016

GCE e2e build/test passed for commit 23571315d351d7a0d71d400d9eff0a2cad47f910.

@k8s-bot
Copy link

k8s-bot commented Apr 17, 2016

GCE e2e build/test passed for commit 78788cecd95bfa4a27777cece9245f903ffff1b0.

@k8s-bot
Copy link

k8s-bot commented Apr 18, 2016

GCE e2e build/test passed for commit b98ce67c528de4ea6c14043ba3be9eb4f8fa2440.

@k8s-bot
Copy link

k8s-bot commented Apr 18, 2016

GCE e2e build/test passed for commit 2dbd2d2685e65cbff26e280353ddc49f46c29a15.

@hongchaodeng
Copy link
Contributor Author

@k8s-bot test node e2e github issue: #24396

var errResult *watch.Event
select {
case err := <-wc.errChan:
errResult = parseError(err)
Copy link
Member

Choose a reason for hiding this comment

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

why not just wc.resultChan <- *parseError(err)

and get rid of errResult completely?

@wojtek-t
Copy link
Member

Just some very minor nit. Other than that LGTM

@hongchaodeng
Copy link
Contributor Author

@wojtek-t Thanks for the review. Addressed and squashed.

@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 18, 2016
@wojtek-t
Copy link
Member

LGTM - thanks!

@k8s-bot
Copy link

k8s-bot commented Apr 18, 2016

GCE e2e build/test passed for commit e18b4e6.

@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 18, 2016

GCE e2e build/test passed for commit e18b4e6.

@k8s-github-robot
Copy link

Automatic merge from submit-queue

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. release-note-none Denotes a PR that doesn't merit a release note. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants