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

Introduce DropPrefix API into Dgraph #3060

Merged
merged 9 commits into from
Feb 27, 2019
Merged

Introduce DropPrefix API into Dgraph #3060

merged 9 commits into from
Feb 27, 2019

Conversation

martinmr
Copy link
Contributor

@martinmr martinmr commented Feb 23, 2019

  • Update version of Badger in the vendor directory.
  • Introduce DropPrefix API into Dgraph
  • Simplify DeleteAll method to use the DropAll method. The reserved predicate data is now deleted entirely and the predicates are recreated afterwards.

This change is Reviewable

manishrjain and others added 2 commits February 22, 2019 13:29
DeleteAll logic now just calls DropAll.

The logic in draft.go takes care of readding the reserved predicates.
@martinmr martinmr requested a review from a team February 23, 2019 01:39
@martinmr martinmr requested review from manishrjain and removed request for a team February 26, 2019 18:45
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 some comments.

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


dgraph/cmd/zero/oracle.go, line 63 at r2 (raw file):

	o.subscribers = make(map[int]chan *pb.OracleDelta)
	o.updates = make(chan *pb.OracleDelta, 100000) // Keeping 1 second worth of updates.
	o.doneUntil.Init(y.NewCloser(0))

Can you add a comment about why y.NewCloser(0) makes sense here. How about passing a nil instead?

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, 1 unresolved discussion (waiting on @manishrjain)


dgraph/cmd/zero/oracle.go, line 63 at r2 (raw file):

Previously, manishrjain (Manish R Jain) wrote…

Can you add a comment about why y.NewCloser(0) makes sense here. How about passing a nil instead?

I created dgraph-io/badger#726 to support passing nil to this function. Once it's merged to master I'll update the vendor directory and change this to nil.

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


query/query4_test.go, line 93 at r2 (raw file):

	setSchema(s1)
	triples := `
		<666> <numerology> "This number is evil"  .

minor: can we also use hexadecimal numbers here to match the numbers used for verification below?


query/query4_test.go, line 136 at r2 (raw file):

	s1 := testSchema + "\n child_pred: uid @reverse .\n"
	setSchema(s1)
	triples := `<666> <child_pred> <777>  .`

switch to hexadecimal numbers?

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.

:lgtm:

Reviewable status: 36 of 40 files reviewed, 3 unresolved discussions (waiting on @manishrjain and @martinmr)

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


query/query4_test.go, line 93 at r2 (raw file):

Previously, gitlw (Lucas Wang) wrote…

minor: can we also use hexadecimal numbers here to match the numbers used for verification below?

Done.


query/query4_test.go, line 136 at r2 (raw file):

Previously, gitlw (Lucas Wang) wrote…

switch to hexadecimal numbers?

Done.

@martinmr martinmr merged commit ddb0d76 into master Feb 27, 2019
@martinmr martinmr deleted the mrjn/drop-prefix branch February 27, 2019 00:12
dna2github pushed a commit to dna2fork/dgraph that referenced this pull request Jul 19, 2019
* Introduce Badger's new DropPrefix API into Dgraph
* Simplify DeleteAll logic. DeleteAll logic now just calls DropAll. The logic in draft.go takes care of readding the reserved predicates.
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