chore(v2): metastore API refactoring #3679
Conversation
kolesnikovae
force-pushed
the
chore/metastore-api-refactoring
branch
from
e745e20
to
346e8c0
Compare
kolesnikovae
force-pushed
the
chore/metastore-api-refactoring
branch
from
4bcf224
to
264a970
Compare
# Conflicts: # go.work.sum
There was a problem hiding this comment.
Looks great to me, thank you for taking the time to do the refactoring.
The only part I am not sure about is how clear some of the new concepts are. In particular, we have the raft_node.Leader and raft_node.Follower types. Their main purpose is ensuring read consistency but their names can be misleading because they are active on both leader and follower nodes. There is also a strong connection between the two, with the follower being the only consumer of what the leader provides (ReadIndex). I wonder if merging them under a StateReader (or StateProvider) is possible and could make their purpose more clear.
| m.observer.OnLeader(m.cleanerService) | ||
| m.observer.OnLeader(m.dlqRecovery) | ||
| m.observer.OnLeader(m.placement) |
There was a problem hiding this comment.
nit, I think here we are registering the 3 services so perhaps the method should reflect that (e.g., Register or RegisterService, ...)
There was a problem hiding this comment.
WDYT about RunOnLeader? We already have quite a few "registrations", and adding some more might not be helpful for the reader
| proposer *RaftProposer | ||
| client metastorev1.RaftNodeServiceClient | ||
| observer *raft_node.Observer | ||
| follower *raft_node.Follower | ||
| leader *raft_node.Leader |
There was a problem hiding this comment.
nit, perhaps prefix all of these with raft to make their usage more clear (e.g., it is not obvious that m.observer refers to the raft state)
There was a problem hiding this comment.
I'm thinking about refactoring raft-related stuff to a separate module: now the metastore type has too many raft-smth members. Will do that in the PR I'm currently working on
| return o | ||
| } | ||
|
|
||
| func (o *Observer) RegisterHandler(h StateHandler) { |
There was a problem hiding this comment.
this can probably be private
There was a problem hiding this comment.
RegisterHandler is actually the primary method. Additionally, I plan to extend the interface to handle notifications for other types of events, such as peer additions or removals, leader loss, and so on
I decided to split #3652 into multiple PRs to avoid bad merge conflicts. In this PR, I refactor the FSM module and gRPC services. The core idea: strict raft command handler interface with storage/FSM injection (
*bbolt.Txfor now). This required quite a few adjustments, but now we always perform all the operations in a single transaction and the state is explicitly provided to the command handler.I tried to preserve as much as possible but decided to not re-write compactor tests as the implementation is changed significantly in #3652.
I'm going to test the PR more extensively (there almost no functional changes), but it is ready for review.