Skip to content
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

[BREAKING] feat(zero): Make zero lease out namespace IDs #7341

Merged
merged 9 commits into from
Jan 27, 2021

Conversation

ahsanbarkati
Copy link
Contributor

@ahsanbarkati ahsanbarkati commented Jan 20, 2021

This PR makes zero lease out the namespaceIDs. It changes the existing AssignUids() API to AssignIds(), which can be used to get both UIDs as well as NamespaceIDs. The lease type which is required is passed with the pb.Num to AssignIds()


This change is Reviewable

Copy link
Contributor

@manishrjain manishrjain left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm: Bunch of comments.

Reviewed 6 of 6 files at r1.
Reviewable status: all files reviewed, 8 unresolved discussions (waiting on @ahsanbarkati and @vvbalaji-dgraph)


dgraph/cmd/zero/assign.go, line 36 at r1 (raw file):

const (
	leaseBandwidth           = uint64(10000)
	leaseTxnTs     leaseType = iota

If you make this public, then the worker package can use the int values of these in pb.Num.


dgraph/cmd/zero/assign.go, line 174 at r1 (raw file):

	} else {
		glog.Errorf("Unknown lease type: %v\n", typ)

return this error.


dgraph/cmd/zero/assign.go, line 179 at r1 (raw file):

}

func (s *Server) AssignUids(ctx context.Context, num *pb.Num) (*pb.AssignedIds, error) {

remove this.


dgraph/cmd/zero/assign.go, line 183 at r1 (raw file):

}

func (s *Server) AssignNsIDs(ctx context.Context, num *pb.Num) (*pb.AssignedIds, error) {

Don't create this func.


dgraph/cmd/zero/assign.go, line 189 at r1 (raw file):

// AssignIds is used to assign new ids (UIDs, NsIDs) by communicating with the leader of the
// RAFT group responsible for handing out ids.
func (s *Server) AssignIds(ctx context.Context, num *pb.Num, typ leaseType) (*pb.AssignedIds, error) {

Make AssignUids public func -> AssignIds. And pass the context via pb.Num. pb.Num isn't persisted, so can use the iota's int value.


protos/pb.proto, line 166 at r1 (raw file):

	License license = 10;
	ZeroSnapshot snapshot = 11; // Used to make Zeros take a snapshot.
	uint64 maxLeaseNsID = 12;

move it next to the other uint64s.


protos/pb.proto, line 182 at r1 (raw file):

	string cid = 8; // Used to uniquely identify the Dgraph cluster.
	License license = 9;
	uint64 maxLeaseNsID = 12;

and here. Use 10.


protos/pb.proto, line 542 at r1 (raw file):

	rpc ShouldServe (Tablet)           returns (Tablet) {}
	rpc AssignUids (Num)               returns (AssignedIds) {}
	rpc AssignNsIDs (Num)              returns (AssignedIds) {}

remove.

Copy link
Contributor

@jarifibrahim jarifibrahim left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I haven't looked at the entire PR. I'll review it again in some time.

Reviewable status: 0 of 20 files reviewed, 18 unresolved discussions (waiting on @ahsanbarkati, @manishrjain, @pawanrawal, and @vvbalaji-dgraph)


dgraph/cmd/alpha/run_test.go, line 1697 at r2 (raw file):

	}
	zc := pb.NewZeroClient(conn)
	if _, err := zc.AssignIds(context.Background(), &pb.Num{Val: 1e6, Type: pb.Num_UID}); err != nil {

100 chars


dgraph/cmd/zero/assign.go, line 40 at r2 (raw file):

	s.Lock()
	s.nextLease[pb.Num_UID] = s.state.MaxUID + 1
	s.nextLease[pb.Num_TXN_TS] = s.state.MaxTxnTs + 1

Does it have to be NUM_TXN_TS? TXN_TS should also suffice.


dgraph/cmd/zero/assign.go, line 44 at r2 (raw file):

	startTs = s.nextLease[pb.Num_TXN_TS]
	glog.Infof("Updated Lease id: %d. Txn Ts: %d. NsID: %d.",

Updated UID


dgraph/cmd/zero/assign.go, line 71 at r2 (raw file):

// so we can tackle any collisions that might happen with the leasemanager
// In essence, we just want one server to be handing out new uids.
func (s *Server) lease(ctx context.Context, num *pb.Num, typ pb.NumLeaseType) (*pb.AssignedIds,

pb.Num already has the type. You don't need to pass the typ parameter.


dgraph/cmd/zero/raft.go, line 416 at r2 (raw file):

	case p.MaxTxnTs > state.MaxTxnTs:
		state.MaxTxnTs = p.MaxTxnTs
	case p.MaxUID != 0 || p.MaxTxnTs != 0:

Don't we need to check the NS ID as well?


graphql/e2e/common/admin.go, line 419 at r2 (raw file):

				}
				maxUID
				maxTxnTs

Add max namespace ID here too.


protos/pb.proto, line 166 at r2 (raw file):

	string cid = 9; // Used as unique identifier for the cluster.
	License license = 10;
	ZeroSnapshot snapshot = 11; // Used to make Zeros take a snapshot.

Add a comment after this line that 12 is already used.


protos/pb.proto, line 182 at r2 (raw file):

	repeated Member removed = 7;
	string cid = 8; // Used to uniquely identify the Dgraph cluster.
	License license = 9;

Add a comment after this line that 10 is already used.


worker/mutation.go, line 509 at r2 (raw file):

	con := pl.Get()
	c := pb.NewZeroClient(con)
	return c.AssignIds(ctx, num)

You should set the num.Type here (I know the caller has set it but it's useful to set it near the function call)

Copy link
Contributor Author

@ahsanbarkati ahsanbarkati left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed comments.

Reviewable status: 0 of 20 files reviewed, 18 unresolved discussions (waiting on @ahsanbarkati, @jarifibrahim, @manishrjain, @pawanrawal, and @vvbalaji-dgraph)


dgraph/cmd/zero/assign.go, line 40 at r2 (raw file):

Previously, jarifibrahim (Ibrahim Jarif) wrote…

Does it have to be NUM_TXN_TS? TXN_TS should also suffice.

NUM_TXN_TS is auto generated by protobuf. It is because TXN_TS is an enum inside the Num message type.

@ahsanbarkati ahsanbarkati changed the title feat(zero): Make zero lease out namespace IDs [BREAKING] feat(zero): Make zero lease out namespace IDs Jan 21, 2021
Copy link
Contributor

@jarifibrahim jarifibrahim left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm:

Reviewable status: 0 of 20 files reviewed, 18 unresolved discussions (waiting on @ahsanbarkati, @jarifibrahim, @manishrjain, @pawanrawal, and @vvbalaji-dgraph)

@netlify
Copy link

netlify bot commented Jan 27, 2021

Deploy preview for dgraph-docs ready!

Built with commit eebbd93

https://deploy-preview-7341--dgraph-docs.netlify.app

@ahsanbarkati ahsanbarkati merged commit 0561801 into master Jan 27, 2021
@joshua-goldstein joshua-goldstein deleted the ahsan/lease-namespaceID branch August 11, 2022 21:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants