| CARVIEW |
Navigation Menu
-
Notifications
You must be signed in to change notification settings - Fork 445
Etcdv3 datasource #115
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
Etcdv3 datasource #115
Conversation
louyuting
added
the
area/data-source
|
@sczyh30 Could you please review this PR? |
Codecov Report
@@ Coverage Diff @@
## master #115 +/- ##
==========================================
- Coverage 42.33% 42.18% -0.15%
==========================================
Files 59 60 +1
Lines 2478 2496 +18
==========================================
+ Hits 1049 1053 +4
- Misses 1309 1324 +15
+ Partials 120 119 -1
Continue to review full report at Codecov.
|
|
I suppose the EtcdDatasourceGenerator is a bit over-designed. Consider to resue etcd client instance through enrich configuration, thus removing the Generator. How about https://github.com/alibaba/sentinel-golang/pull/116/files#diff-d77ed18801d12e79f4e8c13153b63c94 ? |
| // Add watch for propertyKey from lastUpdatedRevision updated after Initializing | ||
| ctx, cancel := context.WithCancel(context.Background()) | ||
| s.cancel = cancel | ||
| rch := s.client.Watch(ctx, s.propertyKey, clientv3.WithCreatedNotify(), clientv3.WithRev(s.lastUpdatedRevision)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You'd better create new watcher by clientv3.NewWatcher(client) instead using s.client.Watcher
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In fact, one WatchClient could watch on multi key range.
Could you please introduce some more details to describe why to create the new watcher for each propertyKey?
The idea is pretty good. I will rethink about generator. |
ext/datasource/etcdv3/etcdv3.go
Outdated
| } | ||
|
|
||
| for _, ev := range resp.Events { | ||
| fmt.Printf("%s %q : %q\n", ev.Type, ev.Kv.Key, ev.Kv.Value) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's for example test, I will remove this log later.
| func (s *Etcdv3DataSource) ReadSource() ([]byte, error) { | ||
| ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second) | ||
| defer cancel() | ||
| resp, err := s.client.Get(ctx, s.propertyKey) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In order to troubleshooting, we should add a info log to log resp
ext/datasource/etcdv3/etcdv3.go
Outdated
| return nil, err | ||
| } | ||
| if resp.Count == 0 { | ||
| return nil, errors.Errorf("The key[%s] is not existed in etcd.", s.propertyKey) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Need to return (nil, nil)
ext/datasource/etcdv3/etcdv3.go
Outdated
| } | ||
|
|
||
| func (s *Etcdv3DataSource) Initialize() error { | ||
| err := s.doReadAndUpdate() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should not return error if parser occurs error when initializing
ext/datasource/etcdv3/etcdv3.go
Outdated
| func (s *Etcdv3DataSource) Initialize() error { | ||
| err := s.doReadAndUpdate() | ||
| if err != nil { | ||
| return err |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here we might need to properly handle the error (e.g. parse error) in case of getting in the way of initialization.
louyuting
force-pushed
the
ext-ds-etcd-v3
branch
from
d946ed3 to
a13df2f
Compare
louyuting
force-pushed
the
ext-ds-etcd-v3
branch
from
a13df2f to
9d2b0d1
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
|
Nice work. Thanks for contributing! |
|
Thanks for @wenxuwan |
Describe what this PR does / why we need it
extend etcdv3 as datasource.
Does this pull request fix one issue?
Describe how you did it
Describe how to verify it
Special notes for reviews