Store group to predicate mapping as part of the backup manifest. #3570
Conversation
There was a problem hiding this comment.
Reviewable status: 0 of 9 files reviewed, 8 unresolved discussions (waiting on @golangcibot, @manishrjain, and @martinmr)
dgraph/cmd/alpha/admin_backup.go, line 113 at r1 (raw file):
} predState, err := worker.PredicateState()
s/predState/state? Unless state was already used above.
dgraph/cmd/zero/zero.go, line 717 at r1 (raw file):
// PredicateState returns the group to predicate mapping. func (s *Server) PredicateState(ctx context.Context, req *pb.Empty) (*pb.MembershipState, error) {
Can use s.membershipState(), which is already available, and then remove whatever you don't need instead of copying what you do need (copying what you do need isn't future-proof, taking it all and removing what you don't is).
Also, no need to have this func in the first place. See my comments below.
dgraph/cmd/zero/zero.go, line 720 at r1 (raw file):
s.RLock() defer s.RUnlock() memState := &pb.MembershipState{
state :=
protos/pb.proto, line 435 at r1 (raw file):
rpc CommitOrAbort (api.TxnContext) returns (api.TxnContext) {} rpc TryAbort (TxnTimestamps) returns (OracleDelta) {} rpc PredicateState(Empty) returns (MembershipState) {}
Don't need this API. Connect gives you the latest membership state. You can ask for info only without causing any changes at Zero.
protos/pb.proto, line 445 at r1 (raw file):
rpc Sort (SortMessage) returns (SortResult) {} rpc Schema (SchemaRequest) returns (SchemaResult) {} rpc Backup (BackupRequest) returns (Empty) {}
How about returning a Status object like Export does?
worker/groups.go, line 947 at r1 (raw file):
zc := pb.NewZeroClient(pl.Get()) out, err := zc.PredicateState(context.Background(), &pb.Empty{})
use Connect endpoint to get the latest membership state.
Technically, the state is already available in groups via the membership stream that's happening. But, I reckon you don't want to use that because it could be behind? Can you confirm that's the reasoning? And add a comment explaining this reasoning.
There was a problem hiding this comment.
Reviewed 9 of 9 files at r1.
Reviewable status: all files reviewed, 8 unresolved discussions (waiting on @golangcibot and @martinmr)
There was a problem hiding this comment.
Reviewable status: all files reviewed, 8 unresolved discussions (waiting on @golangcibot and @manishrjain)
dgraph/cmd/alpha/admin_backup.go, line 113 at r1 (raw file):
Previously, manishrjain (Manish R Jain) wrote…
s/predState/state? Unless state was already used above.
Done.
dgraph/cmd/zero/zero.go, line 717 at r1 (raw file):
Previously, manishrjain (Manish R Jain) wrote…
Can use
s.membershipState(), which is already available, and then remove whatever you don't need instead of copying what you do need (copying what you do need isn't future-proof, taking it all and removing what you don't is).Also, no need to have this func in the first place. See my comments below.
Done. Removed this method.
dgraph/cmd/zero/zero.go, line 720 at r1 (raw file):
Previously, manishrjain (Manish R Jain) wrote…
state :=
Done. Removed this method.
protos/pb.proto, line 435 at r1 (raw file):
Previously, manishrjain (Manish R Jain) wrote…
Don't need this API. Connect gives you the latest membership state. You can ask for info only without causing any changes at Zero.
Done.
protos/pb.proto, line 445 at r1 (raw file):
Previously, manishrjain (Manish R Jain) wrote…
How about returning a Status object like Export does?
Done.
worker/groups.go, line 947 at r1 (raw file):
Previously, manishrjain (Manish R Jain) wrote…
use Connect endpoint to get the latest membership state.
Technically, the state is already available in groups via the membership stream that's happening. But, I reckon you don't want to use that because it could be behind? Can you confirm that's the reasoning? And add a comment explaining this reasoning.
Yes, that's why I was requesting the membership status instead of using the one already in the worker. I have added a comment.
I found UpdateMembershipState, which is already using Connect under the hood to only get the state without updating zero. I have deleted all the PredicateState methods and instead just get the membership status right after the update. This simplified the code quite a bit.
There was a problem hiding this comment.
Do the proto.Clone for membership state. And in another PR, add a ChooseKey func to only choose the right preds picked up by the membership state during backup.
Reviewed 7 of 8 files at r2, 1 of 1 files at r3.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @golangcibot and @martinmr)
worker/groups.go, line 241 at r3 (raw file):
g.RLock() defer g.RUnlock() return g.state
Do the cloning like it is happening in zero.go:membershipState().
There was a problem hiding this comment.
Reviewable status: 8 of 9 files reviewed, 5 unresolved discussions (waiting on @golangcibot and @manishrjain)
dgraph/cmd/alpha/admin_backup.go, line 113 at r2 (raw file):
Previously, golangcibot (Bot from GolangCI) wrote…
File is not
gofmt-ed with-s(fromgofmt)
Done.
dgraph/cmd/alpha/admin_backup.go, line 119 at r2 (raw file):
Previously, golangcibot (Bot from GolangCI) wrote…
nilness: impossible condition: nil != nil (from
govet)
Done.
worker/groups.go, line 241 at r3 (raw file):
Previously, manishrjain (Manish R Jain) wrote…
Do the cloning like it is happening in zero.go:membershipState().
Done.
This change is