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

Implement DropData operation. #3271

Merged
merged 6 commits into from
Apr 18, 2019
Merged

Implement DropData operation. #3271

merged 6 commits into from
Apr 18, 2019

Conversation

martinmr
Copy link
Contributor

@martinmr martinmr commented Apr 9, 2019

This new operation acts as a DropAll except the schema and types are
left intact. Useful when the schema and types will be resued and/or the
schema is large enough that there will be a delay recreating it.

This feature is motivated by
#3236.


This change is Reviewable

This new operation acts as a DropAll except the schema and types are
left intact. Useful when the schema and types will be resued and/or the
schema is large enough that there will be a delay recreating it.

This feature is motivated by
#3236.
@martinmr martinmr requested a review from a team April 9, 2019 00:27
@martinmr
Copy link
Contributor Author

martinmr commented Apr 9, 2019

I copied the pb.go file from dgo in the meantime. I will revert those changes and properly version dgo once dgraph-io/dgo#53 is merged.

Copy link
Contributor

@srfrog srfrog left a comment

Choose a reason for hiding this comment

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

IMO this should be an enum and deprecate drop_all. Have you considered that?
e.g.,

enum DropOp {
  NONE = 0;
  ALL = 1;
  SCHEMA = 2;
  DATA = 3;
}
DropOp drop_op = 8;

Reviewable status: 0 of 12 files reviewed, all discussions resolved

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.

I don't want to deprecate existing features because of this, which will probably be mostly used during tests.

Reviewable status: 0 of 12 files reviewed, all discussions resolved

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.

Same comment as @srfrog, which I arrived independently below. It does seem like an enum would make sense.

Reviewed 12 of 12 files at r1.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @martinmr)


gql/mutation.go, line 39 at r1 (raw file):

	Del      []*api.NQuad
	DropAll  bool
	DropData bool

Does it make sense to make Drop an enum. So, it can tell what to drop? I can think of other scenarios in the future, where you'd want to drop only the types or only the schema or other things.

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: 4 of 26 files reviewed, 1 unresolved discussion (waiting on @manishrjain)


gql/mutation.go, line 39 at r1 (raw file):

Previously, manishrjain (Manish R Jain) wrote…

Does it make sense to make Drop an enum. So, it can tell what to drop? I can think of other scenarios in the future, where you'd want to drop only the types or only the schema or other things.

Done. I used oneof instead of an enum. It has the advantage that it will force only one of the values inside to be set and it let's us keep the current HTTP interface without having to make any changes.

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.

Using enums now but kept drop_all and drop_attr for now for backwards compatibility.

Reviewable status: 5 of 12 files reviewed, 1 unresolved discussion (waiting on @manishrjain)

@martinmr martinmr dismissed manishrjain’s stale review April 11, 2019 17:52

Addressed comments

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: Got a few comments. Address before merging.

Reviewed 8 of 8 files at r2.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @manishrjain and @martinmr)


edgraph/server.go, line 319 at r2 (raw file):

	// if it lies on some other machine. Let's get it for safety.
	m := &pb.Mutations{StartTs: State.getTimestamp(false)}
	if op.DropAll || op.DropOp == api.Operation_ALL {

Given how often this is being used, might make sense to have a helper func isDropAll(op).


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

	if src.DropOp == pb.Mutations_ALL {
		for _, gid := range groups().KnownGroups() {

There's no need to copy the loop.

if src.DropOp == all || src.DropOp == data {
 ...
 mu.DropOp = src.DropOp
}

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: 7 of 11 files reviewed, 3 unresolved discussions (waiting on @manishrjain)


edgraph/server.go, line 319 at r2 (raw file):

Previously, manishrjain (Manish R Jain) wrote…

Given how often this is being used, might make sense to have a helper func isDropAll(op).

Done.


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

Previously, manishrjain (Manish R Jain) wrote…

There's no need to copy the loop.

if src.DropOp == all || src.DropOp == data {
 ...
 mu.DropOp = src.DropOp
}

Done.

@martinmr martinmr merged commit c89560b into master Apr 18, 2019
@martinmr martinmr deleted the martinmr/drop-data branch April 18, 2019 18:56
dna2github pushed a commit to dna2fork/dgraph that referenced this pull request Jul 19, 2019
This new operation acts as a DropAll except the schema and types are
left intact. Useful when the schema and types will be resued and/or the
schema is large enough that there will be a delay recreating it.

This feature is motivated by
dgraph-io#3236
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