-
Notifications
You must be signed in to change notification settings - Fork 9.7k
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
mvcc: sending events after restore #8412
Conversation
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.
general approach looks OK
mvcc/watchable_store_test.go
Outdated
@@ -26,6 +26,8 @@ import ( | |||
"github.com/coreos/etcd/lease" | |||
"github.com/coreos/etcd/mvcc/backend" | |||
"github.com/coreos/etcd/mvcc/mvccpb" | |||
"io/ioutil" |
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.
these should be grouped with the standard libraries above instead of with external packages
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.
The foolish IDE "helped" me did this. Yes, I will corrent this.
mvcc/watchable_store.go
Outdated
func (s *watchableStore) Restore(b backend.Backend) error { | ||
var err error | ||
var needSync bool | ||
func() { |
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.
having a func() {} ()
inside a function is unusual, try:
func (s *watchableStore) Restore(b backend.Backend) error {
s.mu.Lock()
defer s.mu.Unlock()
if err := s.store.Restore(b); err != nil {
return err
}
for wa := range as.synced.watches {
s.unsynced.watchers.add(wa)
}
s.synced = newWatcherGroup()
return nil
}
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 because I have to release the mutex
before calling syncWatchers
that I use func() {} ()
, but as you commented, It will be better not calling syncWatchers
. so, yes, the codes you posted here is better.
mvcc/watchable_store.go
Outdated
needSync = true | ||
}() | ||
if needSync { | ||
s.syncWatchers() |
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.
syncWatchersLoop
will pick this up; there'll already be a pause from restoring, so there's no much of an advantage calling it here
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.
Yes , it is reasonable.
mvcc/watchable_store_test.go
Outdated
} | ||
newPath := filepath.Join(dir, "database_new") | ||
|
||
newBeckend := backend.NewDefaultBackend(newPath) |
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.
newBackend, newPath := backend.NewDefaultTmpBackend()
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.
I am afraid we should specify a new file to create a db here, otherwise the old backend and the new one will use the same file. will there no problem with this?
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.
but NewDefaultTmpBackend
uses ioutil.TempDir
so it will still be a fresh file, right?
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.
yes, I thought it is the same dir. ok I will change it
mvcc/watchable_store_test.go
Outdated
func TestWatchRestoreRevs(t *testing.T) { | ||
b, tmpPath := backend.NewDefaultTmpBackend() | ||
s := newWatchableStore(b, &lease.FakeLessor{}, nil) | ||
defer func() { |
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.
defer cleanup(s, b, tmpPath)
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.
ok
mvcc/watchable_store_test.go
Outdated
|
||
newBeckend := backend.NewDefaultBackend(newPath) | ||
newStore := newWatchableStore(newBeckend, &lease.FakeLessor{}, nil) | ||
defer func() { |
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.
defer cleanup(newStore, newBackend, newPath)
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.
ok
mvcc/watchable_store_test.go
Outdated
@@ -296,6 +298,52 @@ func TestWatchFutureRev(t *testing.T) { | |||
} | |||
} | |||
|
|||
func TestWatchRestoreRevs(t *testing.T) { |
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.
just TestWatchRestore
?
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.
ok
@abel-von Thanks for fixing this! |
5e0d15a
to
52f4118
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 after squashing into a single commit. Thanks!
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. thanks!
@abel-von @heyitsanthony We want to include this in today's release. I will cherry-pick your patch with correct file permission bits today. |
Fixes: #8411
Contributing guidelines
Please read our contribution workflow before submitting a pull request.