etcdserver, raft: cleanup raft.Storage interface and raft.MemoryStora…#8679
etcdserver, raft: cleanup raft.Storage interface and raft.MemoryStora…#8679mitake wants to merge 1 commit into
Conversation
|
@gyuho why? Separating would make the first commit not buildable? |
|
I was worrying projects importing only raft packages, but it shouldn't be a problem. Never mind. |
|
@gyuho I see. But putting changes in a single commit wouldn't be a problem for the case, I think (e.g. clientv3 is often changed with other packages). If it will cause troubles, of course I'll separate the commit. |
mitake
force-pushed
the
raft-storage-cleanup
branch
from
b685695 to
dfa18c1
Compare
mitake
force-pushed
the
raft-storage-cleanup
branch
4 times, most recently
from
0a033f2 to
071a38b
Compare
|
Rebased on the latest master. I couldn't reproduce the gosimple error https://travis-ci.org/coreos/etcd/jobs/336420586 on my local env. Also I can't see errors of the failed CI https://semaphoreci.com/coreos/etcd/branches/pull-request-8679/builds/6 ... |
|
Defer to @xiang90 |
|
@gyuho could you trigger travis CI again? I can't reproduce the error on my local environment, it is strange... |
|
@mitake Can you rebase from master? And force push? We upgraded Go version in travis as well. Thanks! |
mitake
force-pushed
the
raft-storage-cleanup
branch
from
071a38b to
2bc8541
Compare
|
@gyuho thanks, rebased |
| // snapshot and call Snapshot later. | ||
| Snapshot() (pb.Snapshot, error) | ||
| // SetHardState saves the current HardState. | ||
| SetHardState(st pb.HardState) error |
There was a problem hiding this comment.
I am not sure if this makes sense for other people. We do not want to enforce how write operations to be handled.
@bdarnell cockroach db probably has its own storage implementation that handles the write related operations in a different way.
There was a problem hiding this comment.
Yeah, Storage and MemoryStorage were separate by design. It's important for us that the Storage interface is read-only and all writes go through the Ready struct. This lets us do a better job of batching different writes together.
This change is also weird in that it adds a bunch of methods to the Storage interface that we'd have to implement, but it doesn't add any non-test callers of them.
There was a problem hiding this comment.
I see, thanks for pointing out the problem. I'll consider how to do it without disrupting existing users of the raft package.
…ge struct This commit lets raft.MemoryStorage be private (the new name is simply memoryStorage) for cleaner separation between packages. MemoryStorage doesn't have global variable fields and external packages use it as an object which implements raft.Storage in many cases. I think the struct doesn't need to be a global thing.
mitake
force-pushed
the
raft-storage-cleanup
branch
from
2bc8541 to
90076a0
Compare
Codecov Report
@@ Coverage Diff @@
## master #8679 +/- ##
=========================================
Coverage ? 75.63%
=========================================
Files ? 365
Lines ? 30720
Branches ? 0
=========================================
Hits ? 23234
Misses ? 5866
Partials ? 1620
Continue to review full report at Codecov.
|
| // snapshot and call Snapshot later. | ||
| Snapshot() (pb.Snapshot, error) | ||
| // SetHardState saves the current HardState. | ||
| SetHardState(st pb.HardState) error |
There was a problem hiding this comment.
Yeah, Storage and MemoryStorage were separate by design. It's important for us that the Storage interface is read-only and all writes go through the Ready struct. This lets us do a better job of batching different writes together.
This change is also weird in that it adds a bunch of methods to the Storage interface that we'd have to implement, but it doesn't add any non-test callers of them.
|
closing this as @bdarnell suggested. |
5 participants
…ge struct
This commit lets raft.MemoryStorage be private (the new name is simply
memoryStorage) for cleaner separation between packages. MemoryStorage
doesn't have global variable fields and external packages use it as an
object which implements raft.Storage in many cases. I think the struct
doesn't need to be a global thing.
I found the interface separation isn't clean enough during reworking #7782. To the best of my knowledge, making MemoryStorage private wouldn't be a problem.