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

Split lists in bulk loader #4875

Merged
merged 1 commit into from
Mar 17, 2020
Merged

Split lists in bulk loader #4875

merged 1 commit into from
Mar 17, 2020

Conversation

martinmr
Copy link
Contributor

@martinmr martinmr commented Feb 28, 2020

This change is Reviewable

Docs Preview: Dgraph Preview

@martinmr martinmr force-pushed the martinmr/bulk-loader-splits branch from 5116631 to 2db6baa Compare March 2, 2020 18:57
@martinmr martinmr changed the base branch from master to martinmr/recursive-splits March 2, 2020 22:36
@martinmr martinmr marked this pull request as ready for review March 3, 2020 22:25
@martinmr martinmr requested review from manishrjain and a team as code owners March 3, 2020 22:25
@martinmr martinmr changed the base branch from martinmr/recursive-splits to master March 11, 2020 19:52
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.

You could have a stream per split key per predicate. So, each pred has its own stream id. You need another stream id per such stream id to represent splits. You could achieve that by using the MSB bit to represent split stream for that predicate.

Reviewable status: 0 of 2 files reviewed, 1 unresolved discussion (waiting on @danielmai, @manishrjain, @martinmr, @MichaelJCompton, and @pawanrawal)

@martinmr martinmr force-pushed the martinmr/bulk-loader-splits branch from 51c2d8f to 1426660 Compare March 17, 2020 00:30
Copy link
Contributor Author

@martinmr martinmr left a comment

Choose a reason for hiding this comment

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

Done using a diff stream id for the split keys.

Reviewable status: 0 of 2 files reviewed, 1 unresolved discussion (waiting on @danielmai, @golangcibot, @MichaelJCompton, and @pawanrawal)


dgraph/cmd/bulk/reduce.go, line 123 at r1 (raw file):

Previously, golangcibot (Bot from GolangCI) wrote…

Error return value of wb.SetEntry is not checked (from errcheck)

Done.

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.

Do ensure that bulk loader runs and outputs a bunch of split keys, before merging.

@balajijinnah Can you look at this carefully? Defer to you for approval.

Reviewable status: 0 of 2 files reviewed, 1 unresolved discussion (waiting on @danielmai, @golangcibot, @MichaelJCompton, and @pawanrawal)

@martinmr
Copy link
Contributor Author

Verified keys show up in the debug tool.

@martinmr martinmr merged commit 3c1ce0e into master Mar 17, 2020
@martinmr martinmr deleted the martinmr/bulk-loader-splits branch March 17, 2020 18:19
martinmr added a commit that referenced this pull request Mar 18, 2020
The equivalent of PR #4875 for the 1.2 branch. The original change
cannot be cherry-picked because there are other changes to the bulk
loader that are not included in this branch.
martinmr added a commit that referenced this pull request Mar 18, 2020
The equivalent of PR #4875 for the 1.2 branch. The original change
cannot be cherry-picked because there are other changes to the bulk
loader that are not included in this branch.
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