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

*: use etcd for privilege update notification #3030

Merged
merged 6 commits into from
Apr 11, 2017

Conversation

tiancaiamao
Copy link
Contributor

PD already have etcd embed, take full use of it.

PD already have etcd embed, take full use of it.
@hanfei1991
Copy link
Member

can you update vendor in another pr ?

@tiancaiamao tiancaiamao changed the base branch from master to tiancaiamao/update-vendor April 11, 2017 02:46
@tiancaiamao
Copy link
Contributor Author

I'll rebase it after #3032

@tiancaiamao tiancaiamao changed the base branch from tiancaiamao/update-vendor to master April 11, 2017 02:52
@tiancaiamao tiancaiamao force-pushed the tiancaiamao/notify-update branch from ff60b6e to 86ba2e2 Compare April 11, 2017 05:04
@@ -0,0 +1,34 @@
// Copyright 2015 PingCAP, Inc.
Copy link
Contributor

Choose a reason for hiding this comment

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

s/2015/2017

session.go Outdated
DialTimeout: 5 * time.Second,
})
if err != nil {
return dom, errors.Trace(err)
Copy link
Contributor

Choose a reason for hiding this comment

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

s/dom/nil

domain/domain.go Outdated
}
case <-do.exit:
return
// if etcd is available, notification i
Copy link
Contributor

Choose a reason for hiding this comment

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

Please complete this comment.

@zimulala
Copy link
Contributor

Do you forget to replace the update for executeFlush?

@coocood
Copy link
Member

coocood commented Apr 11, 2017

We don't need to make a notify package, notify should be a method of Domain.

@tiancaiamao
Copy link
Contributor Author

PTAL @coocood @hanfei1991 @zimulala

@coocood
Copy link
Member

coocood commented Apr 11, 2017

LGTM

coocood
coocood previously approved these changes Apr 11, 2017
@zimulala
Copy link
Contributor

LGTM

@tiancaiamao tiancaiamao merged commit 654c10f into master Apr 11, 2017
@tiancaiamao tiancaiamao deleted the tiancaiamao/notify-update branch April 11, 2017 11:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants