refreshable file data source #86
Conversation
louyuting
force-pushed
the
datasource-ext-freshable_file
branch
from
6ab8eda
to
6e51178
Compare
louyuting
added
area/data-source
louyuting
force-pushed
the
datasource-ext-freshable_file
branch
2 times, most recently
from
c6b1fc1
to
5a69066
Compare
| } | ||
|
|
||
| func (s *RefreshableFileDataSource) Close() error { | ||
| return nil |
There was a problem hiding this comment.
The Close op should be supported.
There was a problem hiding this comment.
Make sense.
The semantic of Close is to close the datasource and stop listen on dynamic datasource.
| watcher *fsnotify.Watcher | ||
| } | ||
|
|
||
| func FileDataSourceStarter(sourceFilePath string, handlers ...datasource.PropertyHandler) (*RefreshableFileDataSource, error) { |
There was a problem hiding this comment.
The name starter might be confusing?
There was a problem hiding this comment.
I will rename to NewFileDataSource
|
|
||
| func (s *RefreshableFileDataSource) ReadSource() ([]byte, error) { | ||
| f, err := os.Open(s.sourceFilePath) | ||
| defer f.Close() |
There was a problem hiding this comment.
This should be put after the error checking?
| defer f.Close() | ||
|
|
||
| if err != nil { | ||
| return nil, errors.Errorf("The rules file is not existed, err: %+v.", errors.WithStack(err)) |
There was a problem hiding this comment.
This may not just indicate non-exist, but also other circumstances like permission denied.
There was a problem hiding this comment.
Make sense, I will refine the statement.
| return nil | ||
| } | ||
|
|
||
| func (s *RefreshableFileDataSource) doUpdate() { |
| for _, h := range s.Handlers() { | ||
| err := h.Handle(src) | ||
| if err != nil { | ||
| logger.Errorf("RefreshableFileDataSource fail to publish rules, handle: %+v; err: %+v.", h, err) |
There was a problem hiding this comment.
(1) Here we could use the word "data" instead of rule. (2) Printing the handler here might not be helpful. Maybe we need to print out the type?
There was a problem hiding this comment.
(1) Right；
(2) Is type the datasource type?
| s.doUpdate() | ||
| w, err := fsnotify.NewWatcher() | ||
| if err != nil { | ||
| return errors.Errorf("Fail to new a w of fsnotify, err: %+v", err) |
There was a problem hiding this comment.
yes, it's caused by rename, I will rename here.
There was a problem hiding this comment.
Yes, it's caused by mis rename. I will fix here.
|
|
||
| func (s *RefreshableFileDataSource) Initialize() error { | ||
| if !s.isInitialized.CompareAndSet(false, true) { | ||
| return errors.New("RefreshableFileDataSource had been Initialized.") |
There was a problem hiding this comment.
Maybe just returning "nil" is enough?
louyuting
force-pushed
the
datasource-ext-freshable_file
branch
from
fee5480
to
407c196
Compare
|
|
||
| func (s *RefreshableFileDataSource) Close() error { | ||
| s.closeChan <- struct{}{} | ||
| logger.Info("The RefreshableFileDataSource had been closed.") |
There was a problem hiding this comment.
Also print the filePath here to make it clear to identify?
louyuting
force-pushed
the
datasource-ext-freshable_file
branch
from
407c196
to
a853dac
Compare
|
Maybe we could also add the case for file-deleted event? |
|
Nice work. Thanks! |
Describe what this PR does / why we need it
refreshable file data source implementation.
enhance the datasource framework:
a) Add return value error for PropertyConverter function to indicate whether occur error when converting.
b)Add return value error for datasource. ReadSource function to indicate whether occur error when reading source.
refreshable file datasource implementation
add plugin module.
a) This module is design to encapsulate converter(default json) and updater related logic.
Does this pull request fix one issue?
Resolves #84
Describe how you did it
Describe how to verify it
Unit test
Special notes for reviews