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

Include types in results of export operation. #3493

Merged
merged 3 commits into from
Jun 3, 2019
Merged

Conversation

martinmr
Copy link
Contributor

@martinmr martinmr commented May 31, 2019

This change is Reviewable

@martinmr martinmr requested review from manishrjain and a team as code owners May 31, 2019 01:38
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: Get it reviewed by another team member as well.

Reviewed 2 of 2 files at r1.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @martinmr)


worker/export.go, line 360 at r1 (raw file):

func fieldToString(update *pb.SchemaUpdate) string {
	var buf bytes.Buffer

Maybe use strings.Builder


worker/export.go, line 471 at r1 (raw file):

	}
	glog.Infof("Exporting schema for group: %d at %s\n", in.GroupId, schemaPath)
	schemaTypeWriter := &fileWriter{}

No need to change the name. Type is part of the schema.


worker/export_test.go, line 198 at r1 (raw file):

	// Index the name predicate. We ensure it doesn't show up on export.
	initTestExport(t, "name:string @index .")
	// initTypes(t)

extra comment?

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.

Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @manishrjain)


worker/export.go, line 360 at r1 (raw file):

Previously, manishrjain (Manish R Jain) wrote…

Maybe use strings.Builder

Done.


worker/export.go, line 471 at r1 (raw file):

Previously, manishrjain (Manish R Jain) wrote…

No need to change the name. Type is part of the schema.

Done.


worker/export_test.go, line 198 at r1 (raw file):

Previously, manishrjain (Manish R Jain) wrote…

extra comment?

Done.

@martinmr martinmr requested a review from gitlw June 1, 2019 01:04
@mangalaman93 mangalaman93 requested a review from manishrjain June 3, 2019 17:50
Copy link
Member

@mangalaman93 mangalaman93 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: 0 of 2 files reviewed, 5 unresolved discussions (waiting on @gitlw, @manishrjain, and @martinmr)


worker/export.go, line 493 at r2 (raw file):

		if !pk.IsType() {
			if servesTablet, err := groups().ServesTablet(pk.Attr); err != nil || !servesTablet {

I was wondering whether a predicate movement completes while export is going on. Can that cause problems?


worker/export_test.go, line 289 at r2 (raw file):

func TestExportJson(t *testing.T) {
	// Index the name predicate. We ensure it doesn't show up on export.

Does this mean we do not export schema for predicates that are not part of type system?

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.

Reviewable status: 0 of 2 files reviewed, 5 unresolved discussions (waiting on @gitlw, @mangalaman93, and @manishrjain)


worker/export.go, line 493 at r2 (raw file):

Previously, mangalaman93 (Aman Mangal) wrote…

I was wondering whether a predicate movement completes while export is going on. Can that cause problems?

There shouldn't be any problems. The export is using it's own read ts so if that read timestamp is from before the predicate move completed, the original group will be exporting the data and the destination group will not see any of it.


worker/export_test.go, line 289 at r2 (raw file):

Previously, mangalaman93 (Aman Mangal) wrote…

Does this mean we do not export schema for predicates that are not part of type system?

No. Name doesn't show up in the export because it's not stored in disk. I think this is intentionally done to make the test verification easier. I added this method here (it's already in the rdf test) for consistency between the two tests. It's got nothing to do with types.

Copy link

@gitlw gitlw 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: 0 of 2 files reviewed, 6 unresolved discussions (waiting on @mangalaman93, @manishrjain, and @martinmr)


worker/export_test.go, line 192 at r2 (raw file):

	// This order will be preserved due to file naming
	require.Equal(t, 3, count)

minor: it seems this testing is not necessary given the testing of len(result.Schemas) and len(result.Types) before.

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.

Reviewable status: 0 of 2 files reviewed, 3 unresolved discussions (waiting on @manishrjain)


worker/export_test.go, line 192 at r2 (raw file):

Previously, gitlw (Lucas Wang) wrote…

minor: it seems this testing is not necessary given the testing of len(result.Schemas) and len(result.Types) before.

Done

@martinmr martinmr merged commit a41db40 into master Jun 3, 2019
@martinmr martinmr deleted the martinmr/export-types branch June 3, 2019 19:05
dna2github pushed a commit to dna2fork/dgraph that referenced this pull request Jul 19, 2019
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.

4 participants