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

Bulk multi-shard DBs fix #3065

Merged
merged 11 commits into from
Feb 26, 2019
Merged

Bulk multi-shard DBs fix #3065

merged 11 commits into from
Feb 26, 2019

Conversation

codexnull
Copy link
Contributor

@codexnull codexnull commented Feb 25, 2019

Change summary:

  • Each shard's schema contains only the predicates found in that shard rather than all preds.

  • Test case for above fix.

  • More user-friendly "file not found" messages rather than stack traces when bulk schema or data paths invalid.


This change is Reviewable

@codexnull codexnull requested a review from a team February 25, 2019 23:28
Copy link
Contributor

@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.

Reviewed 4 of 4 files at r2.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @codexnull)


dgraph/cmd/bulk/loader.go, line 246 at r2 (raw file):

m 

rename this variable to something a bit more descriptive. m implies a map but we are using this more as a set.

Ideas: usedPreds, fullPreds, seenPreds or something like that.


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

		os.Exit(1)
	} else if _, err := os.Stat(opt.DataFiles); err != nil && os.IsNotExist(err) {
		fmt.Fprintf(os.Stderr, "Data path(%v) does not exist .\n", opt.DataFiles)

Remove the blank space between exist and the period.

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: Merge after addressing @martinmr 's comments.

Reviewed 2 of 4 files at r2, 2 of 2 files at r3.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @codexnull)

Copy link
Contributor Author

@codexnull codexnull left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @martinmr)


dgraph/cmd/bulk/loader.go, line 246 at r2 (raw file):

Previously, martinmr (Martin Martinez Rivera) wrote…
m 

rename this variable to something a bit more descriptive. m implies a map but we are using this more as a set.

Ideas: usedPreds, fullPreds, seenPreds or something like that.

I used m because it matches ld.schema.m below.


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

Previously, martinmr (Martin Martinez Rivera) wrote…

Remove the blank space between exist and the period.

Done.

@codexnull codexnull merged commit 81ff46e into master Feb 26, 2019
codexnull added a commit that referenced this pull request Feb 27, 2019
Change summary:

* Each shard's schema contains only the predicates found in that shard rather than all preds.

* Test case for above fix.

* More user-friendly "file not found" messages rather than stack traces when bulk schema or data paths incorrect.
@codexnull codexnull deleted the javier/bulk_schema branch March 1, 2019 22:53
dna2github pushed a commit to dna2fork/dgraph that referenced this pull request Jul 19, 2019
Change summary:

 * Each shard's schema contains only the predicates found in that shard rather than all preds.

* Test case for above fix.

* More user-friendly "file not found" messages rather than stack traces when bulk schema or data paths incorrect.
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