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

Don't read data posting lists in certain cases #3713

Merged
merged 4 commits into from
Jul 24, 2019

Conversation

manishrjain
Copy link
Contributor

@manishrjain manishrjain commented Jul 23, 2019

To further #3695 , only read posting lists in certain cases. For e.g., when they are indexed, or when there's a delete operation or when it's a single UID predicate, etc. For the rest, don't read them. This further improves the performance of live data loading.


This change is Reviewable

@manishrjain manishrjain requested review from martinmr and a team as code owners July 23, 2019 21:52
@manishrjain manishrjain requested a review from gitlw July 23, 2019 21:52
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 2 of 2 files at r1.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @gitlw and @manishrjain)


posting/oracle.go, line 80 at r1 (raw file):

}

func (txn *Txn) GetFromDelta(key []byte) (*List, error) {

add docstring


worker/mutation.go, line 82 at r1 (raw file):

Quoted 6 lines of code…
	// The following is a performance optimization which allows us to not read a posting list from
	// disk. We calculate this based on how AddMutationWithIndex works. General idea is that if
	// we're not using the read posting list, we don't need to retrieve it. We need posting list if
	// we're doing indexing or count index or enforcing single UID, etc. In other cases, we can just
	// create a posting list facade in memory and use it to store the delta in Badger. Later, Rollup
	// operation would consolidate all these deltas into a posting list.

minor:

"The general idea"
"We need the posting list"
"Later, the Rollup operation"


worker/mutation.go, line 85 at r1 (raw file):

	// create a posting list facade in memory and use it to store the delta in Badger. Later, Rollup
	// operation would consolidate all these deltas into a posting list.
	var fn func(key []byte) (*posting.List, error)

maybe call this getFn as fn is too generic?


worker/mutation.go, line 99 at r1 (raw file):

		// Reverse index doesn't need the posting list to be read. We already covered count index,
		// single uid and delete all above.
		// Values, whether single or list, don't need to read.

minor: "to be read"


worker/mutation.go, line 100 at r1 (raw file):

		// single uid and delete all above.
		// Values, whether single or list, don't need to read.
		// Uid list doesn't need to read.

minor: "to be read"

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: after addressing Martin's comments.

Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @gitlw and @manishrjain)

Copy link
Contributor Author

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

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


posting/oracle.go, line 80 at r1 (raw file):

Previously, martinmr (Martin Martinez Rivera) wrote…

add docstring

Done.


worker/mutation.go, line 82 at r1 (raw file):

Previously, martinmr (Martin Martinez Rivera) wrote…
	// The following is a performance optimization which allows us to not read a posting list from
	// disk. We calculate this based on how AddMutationWithIndex works. General idea is that if
	// we're not using the read posting list, we don't need to retrieve it. We need posting list if
	// we're doing indexing or count index or enforcing single UID, etc. In other cases, we can just
	// create a posting list facade in memory and use it to store the delta in Badger. Later, Rollup
	// operation would consolidate all these deltas into a posting list.

minor:

"The general idea"
"We need the posting list"
"Later, the Rollup operation"

Done.


worker/mutation.go, line 85 at r1 (raw file):

Previously, martinmr (Martin Martinez Rivera) wrote…

maybe call this getFn as fn is too generic?

Done.

@manishrjain manishrjain merged commit b177deb into master Jul 24, 2019
@manishrjain manishrjain deleted the mrjn/smart-list-retrieval branch July 24, 2019 17:57
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