Bulk loader allocates reserved predicates in first reduce shard. #4202

martinmr · 2019-10-22T00:53:24Z

This PR contains a couple of related changes.

Bulk loader forces reserved predicates to end up in the first reduce shard. It
also writes a file in the posting directories with the proposed group ID for
each shard.
Dgraph looks at that file during startup and uses it to request the right group
ID from zero.

The change is being tested by modifying the 21million test to use multiple groups
and add a new query to verify the number of nodes with a dgraph.type predicate.
If the test runs without the fix, dgraph.type sometime ends up in a different group.

Fixes #3968.

This change is

pullrequest

✅ A review job has been created and sent to the PullRequest network.

@martinmr you can click here to see the review status or cancel the code review job.

pullrequest

	1 Message
✅	It looks like the description for this pull request is either blank or very short. Adding a high-level summary will help our reviewers provide better feedback. Feel free to include questions for PullRequest reviewers and make specific feedback requests.

pullrequest

Just one minor comment on the raft id conditional. Rest are readability.

Reviewed with ❤️ by PullRequest

edgraph/server.go

worker/groups.go

dgraph/cmd/bulk/merge_shards.go

dgraph/cmd/bulk/run.go

output was assumed to be sorted but that assumption is wrong. Fix is to explicitly sort.

martinmr

Reviewable status: 0 of 9 files reviewed, 6 unresolved discussions (waiting on @golangcibot and @pullrequest[bot])

dgraph/cmd/bulk/merge_shards.go, line 35 at r1 (raw file):

Previously, pullrequest[bot] wrote…

let's rename mapShards to shards, and shardDirs to readShardDirs. This will help us with readability and logging at line 52.

Done.

dgraph/cmd/bulk/merge_shards.go, line 52 at r1 (raw file):

Previously, pullrequest[bot] wrote…

this could print fmt.Printf("Reduced MapShards %v to %s\n", shardDirs, reduceShard so we can see what reduced to what

Output is this way because shards are not reduced all at once.

dgraph/cmd/bulk/run.go, line 199 at r2 (raw file):

Previously, golangcibot (Bot from GolangCI) wrote…

File is not gofmt-ed with -s (from gofmt)
		x.Check2(f.WriteString(strconv.Itoa(i + 1)))

Done.

edgraph/server.go, line 106 at r1 (raw file):

Previously, pullrequest[bot] wrote…

You could also post the contents here to help more with debugging.

File might be long or contain special characters. Path to the file is already included so users could inspect the contents themselves.

worker/groups.go, line 100 at r1 (raw file):

Previously, pullrequest[bot] wrote…

Should this check be outside of the x.WorkerConfig.RaftId == 0 check? if RaftId is already non-zero, then the groupId will not be 0.

This is the right place. The check is to prevent servers from using the group id in the file if they already have RAFT information of their own.

worker/groups.go, line 109 at r1 (raw file):

Previously, pullrequest[bot] wrote…

Let's add tests to ensure that the ProposedGroupId is propagated to the GroupId field, and not if the raft id info exists.

Modified an existing test to check this PR works as expected.

manishrjain

Reviewed it. But, please look at the comments and get it approved by @pawanrawal .

Reviewed 1 of 6 files at r1.
Reviewable status: 1 of 10 files reviewed, 10 unresolved discussions (waiting on @golangcibot, @manishrjain, @martinmr, and @pullrequest[bot])

edgraph/server.go, line 106 at r4 (raw file):

	contents, err := ioutil.ReadFile(filepath.Join(Config.PostingDir, groupFile))
	if err != nil {
		glog.Warningf("%s file not found or could not be opened", groupFile)

Dont' spit out anything.

edgraph/server.go, line 109 at r4 (raw file):

		return
	}

message here. Info log. Found a group_id file. Attempting ...

edgraph/server.go, line 110 at r4 (raw file):

	}

	groupId, err := strconv.ParseUint(strings.TrimSpace(string(contents)), 10, 32)

ParseUint(, 0, 32)

edgraph/server.go, line 111 at r4 (raw file):

	groupId, err := strconv.ParseUint(strings.TrimSpace(string(contents)), 10, 32)
	x.Checkf(err, "Error reading %s file inside posting directory %s",

Don't check fail. Spit out an error and just say it is going to be ignored.

worker/groups.go, line 109 at r1 (raw file):

Previously, martinmr (Martin Martinez Rivera) wrote…

Modified an existing test to check this PR works as expected.

check multiple things here:

If the group doesnt exist in Zero, do you get assigned to it.
If the group is already over capacity, do you get assigned to it.

golangcibot · 2019-10-23T20:49:59Z

edgraph/server.go

+
+	glog.Infof("Found group_id file inside posting directory %s. Will attempt to read.",
+		Config.PostingDir)
+	groupId, err := strconv.ParseUint(strings.TrimSpace(string(contents)), 0, 32)


ineffectual assignment to err (from ineffassign)

martinmr

Reviewable status: 1 of 13 files reviewed, 11 unresolved discussions (waiting on @golangcibot, @manishrjain, and @pullrequest[bot])

edgraph/server.go, line 106 at r4 (raw file):

Previously, manishrjain (Manish R Jain) wrote…

Dont' spit out anything.

Done.

edgraph/server.go, line 109 at r4 (raw file):

Previously, manishrjain (Manish R Jain) wrote…

message here. Info log. Found a group_id file. Attempting ...

Done.

edgraph/server.go, line 110 at r4 (raw file):

Previously, manishrjain (Manish R Jain) wrote…

ParseUint(, 0, 32)

Done.

edgraph/server.go, line 111 at r4 (raw file):

Previously, manishrjain (Manish R Jain) wrote…

Don't check fail. Spit out an error and just say it is going to be ignored.

Done.

edgraph/server.go, line 111 at r5 (raw file):

Previously, golangcibot (Bot from GolangCI) wrote…

ineffectual assignment to err (from ineffassign)

Done.

worker/groups.go, line 109 at r1 (raw file):

Previously, manishrjain (Manish R Jain) wrote…

check multiple things here:

If the group doesnt exist in Zero, do you get assigned to it.

If the group is already over capacity, do you get assigned to it.

If the group doesn't exist, Zero proposes creating the group with the alpha as its first member. This is the behavior we want.
If the group is over capacity, it will be assigned to another group with capacity. I have changed the behavior to force the alpha in the group if the group id comes from the file.

pawanrawal

I am assuming that you have verified the fix on the original data set mentioned in the issue? I have some comments, please address them and then this should be good to go.

Reviewed 2 of 6 files at r1, 2 of 4 files at r2, 3 of 4 files at r4, 1 of 2 files at r5, 5 of 5 files at r6.
Reviewable status: all files reviewed, 14 unresolved discussions (waiting on @golangcibot, @manishrjain, @martinmr, and @pullrequest[bot])

dgraph/cmd/bulk/merge_shards.go, line 37 at r6 (raw file):

	shardDirs := readShardDirs(filepath.Join(opt.TmpDir, mapShardDir))
	// First shard is handled differently because it contains reserved predicates.
	firstShard := shardDirs[0]

Are we sure, we'll have atleast one element in the slice always?

protos/pb.proto, line 133 at r6 (raw file):

	bool cluster_info_only = 13;
  bool force_group_id = 14;

The indentation is messed up.

worker/groups.go, line 110 at r6 (raw file):

	// Connect with Zero leader and figure out what group we should belong to.
	m := &pb.Member{Id: x.WorkerConfig.RaftId, GroupId: x.WorkerConfig.ProposedGroupId,
		Addr: x.WorkerConfig.MyAddr, ForceGroupId: true}

Shouldn't ForceGroupId only be true if ProposedGroupId > 0? What happens if a alpha node restarts, we would send GroupId as 0 (we should be sending the actual group id in that case) and ForceGroupId as true. Doesn't that cause issues?

martinmr

Reviewable status: 10 of 13 files reviewed, 14 unresolved discussions (waiting on @golangcibot, @manishrjain, @pawanrawal, and @pullrequest[bot])

dgraph/cmd/bulk/merge_shards.go, line 37 at r6 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

Are we sure, we'll have atleast one element in the slice always?

Yes, there should be at least one map shard to be merged into the reduced shards. I added an assert just in case.

protos/pb.proto, line 133 at r6 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

The indentation is messed up.

Done.

worker/groups.go, line 110 at r6 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

Shouldn't ForceGroupId only be true if ProposedGroupId > 0? What happens if a alpha node restarts, we would send GroupId as 0 (we should be sending the actual group id in that case) and ForceGroupId as true. Doesn't that cause issues?

It's fine right now because the field is only used if the group ID is > 0. But I am following your suggestion just for more robustness.

martinmr

I didn't use the original data set. It doesn't really matter. The issue happens because of the presence of dgraph.type predicates. I used the 21 million dataset to test this, which contains a lot of those predicates.

Reviewable status: 10 of 13 files reviewed, 14 unresolved discussions (waiting on @golangcibot, @manishrjain, @pawanrawal, and @pullrequest[bot])

pawanrawal

Reviewed 3 of 3 files at r7.
Reviewable status: all files reviewed, 14 unresolved discussions (waiting on @golangcibot, @manishrjain, @pawanrawal, and @pullrequest[bot])

martinmr added 2 commits October 21, 2019 16:14

First commit .

08bbad5

store group_id in files and use to propose group membership.

7df740c

pullrequest bot reviewed Oct 22, 2019

View reviewed changes

edgraph/server.go Outdated Show resolved Hide resolved

worker/groups.go Show resolved Hide resolved

worker/groups.go Show resolved Hide resolved

dgraph/cmd/bulk/merge_shards.go Show resolved Hide resolved

dgraph/cmd/bulk/merge_shards.go Outdated Show resolved Hide resolved

modify 21million test to have multiple groups.

bb48e00

golangcibot reviewed Oct 22, 2019

View reviewed changes

dgraph/cmd/bulk/run.go Outdated Show resolved Hide resolved

martinmr added 2 commits October 22, 2019 13:57

Fix bug in shardDirs

8757e06

output was assumed to be sorted but that assumption is wrong. Fix is to explicitly sort.

go fmt.

2e2f720

martinmr commented Oct 22, 2019

View reviewed changes

martinmr added 2 commits October 22, 2019 14:37

Address review comments.

4003ef8

Fix formatting.

84e162c

martinmr changed the title ~~Fix #3968~~ Bulk loader allocates reserved predicates in first reduce shard. Oct 22, 2019

martinmr marked this pull request as ready for review October 22, 2019 22:20

martinmr requested review from manishrjain and a team as code owners October 22, 2019 22:20

manishrjain suggested changes Oct 23, 2019

View reviewed changes

Address review comments.

309855d

golangcibot reviewed Oct 23, 2019

View reviewed changes

martinmr added 2 commits October 23, 2019 14:27

Fix error handling.

60958e4

Force alpha to be in proposed group.

9bb2947

martinmr commented Oct 23, 2019

View reviewed changes

martinmr requested review from manishrjain and pawanrawal October 23, 2019 21:41

pawanrawal suggested changes Oct 24, 2019

View reviewed changes

address review comments.

a9dc59c

martinmr commented Oct 24, 2019

View reviewed changes

martinmr requested a review from pawanrawal October 24, 2019 17:25

martinmr commented Oct 24, 2019

View reviewed changes

pawanrawal reviewed Oct 24, 2019

View reviewed changes

martinmr merged commit 2813b34 into master Oct 24, 2019

martinmr deleted the martinmr/bulk-reserved-preds branch October 24, 2019 22:46

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bulk loader allocates reserved predicates in first reduce shard. #4202

Bulk loader allocates reserved predicates in first reduce shard. #4202

martinmr commented Oct 22, 2019 •

edited

Loading

pullrequest bot left a comment

pullrequest bot left a comment •

edited

Loading

pullrequest bot left a comment

martinmr left a comment

manishrjain left a comment

golangcibot Oct 23, 2019

martinmr left a comment

pawanrawal left a comment

martinmr left a comment

martinmr left a comment

pawanrawal left a comment

Bulk loader allocates reserved predicates in first reduce shard. #4202

Bulk loader allocates reserved predicates in first reduce shard. #4202

Conversation

martinmr commented Oct 22, 2019 • edited Loading

pullrequest bot left a comment

Choose a reason for hiding this comment

pullrequest bot left a comment • edited Loading

Choose a reason for hiding this comment

pullrequest bot left a comment

Choose a reason for hiding this comment

martinmr left a comment

Choose a reason for hiding this comment

manishrjain left a comment

Choose a reason for hiding this comment

golangcibot Oct 23, 2019

Choose a reason for hiding this comment

martinmr left a comment

Choose a reason for hiding this comment

pawanrawal left a comment

Choose a reason for hiding this comment

martinmr left a comment

Choose a reason for hiding this comment

martinmr left a comment

Choose a reason for hiding this comment

pawanrawal left a comment

Choose a reason for hiding this comment

martinmr commented Oct 22, 2019 •

edited

Loading

pullrequest bot left a comment •

edited

Loading