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

feat(Dgraph): Add experimental cache for posting lists #6245

Merged
merged 10 commits into from
Aug 24, 2020

Conversation

martinmr
Copy link
Contributor

@martinmr martinmr commented Aug 19, 2020

Add a cache for posting lists. There are still some issues (i.e failing Jepsen tests) so this feature
is being controlled by a flag for now and disabled by default.


This change is Reviewable

Docs Preview: Dgraph Preview

Bring over changes from balaji/cache.
@martinmr martinmr marked this pull request as draft August 19, 2020 21:06
@martinmr martinmr marked this pull request as ready for review August 20, 2020 18:56
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.

Get it reviewed by @balajijinnah as well. Looks alright to me, have a few comments.

Reviewed 8 of 8 files at r1.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @martinmr and @vvbalaji-dgraph)


dgraph/cmd/alpha/run.go, line 203 at r1 (raw file):

	// Flag to enable posting list Ristretto cache.
	flag.Bool("pl_cache", false, "EXPERIMENTAL. Set to true to enable the posting list cache. "+

pl_cachemb


posting/lists.go, line 168 at r1 (raw file):

		for range ticker.C {
			ostats.Record(context.Background(),
				x.CacheInUse.M(int64(m.CostAdded()-m.CostEvicted())),

This would be tricky because we'll have many caches -- also from Badger.


posting/mvcc.go, line 268 at r1 (raw file):

// RemoveCacheFor will delete the list corresponding to the given key.
func RemoveCacheFor(key []byte) {
	lCache.Del(key)

Maybe not Del, instead do Set.


posting/mvcc.go, line 433 at r1 (raw file):

		return l, err
	}
	lCache.Set(key, l, 0)

The cost should never be zero.

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, 4 unresolved discussions (waiting on @balajijinnah, @manishrjain, and @vvbalaji-dgraph)


dgraph/cmd/alpha/run.go, line 203 at r1 (raw file):

Previously, manishrjain (Manish R Jain) wrote…

pl_cachemb

Done. If I am understanding this correctly, adding "mb" means we should pass the size of the cache in this flag and disable the cache if the value is zero, right?


posting/lists.go, line 168 at r1 (raw file):

Previously, manishrjain (Manish R Jain) wrote…

This would be tricky because we'll have many caches -- also from Badger.

Ok. I have removed the metrics for now and added a log instead that prints the metrics once every minute.


posting/mvcc.go, line 268 at r1 (raw file):

Previously, manishrjain (Manish R Jain) wrote…

Maybe not Del, instead do Set.

Not sure if this is correct. What if the key is evicted, then we wouldn't update immediately.
@balajijinnah, what do you think?


posting/mvcc.go, line 433 at r1 (raw file):

Previously, manishrjain (Manish R Jain) wrote…

The cost should never be zero.

If it's zero, Ristretto will use the default cost function.

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: Added some comments, please address.

Reviewed 7 of 7 files at r2.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @martinmr and @vvbalaji-dgraph)


dgraph/cmd/alpha/run.go, line 203 at r1 (raw file):

Previously, manishrjain (Manish R Jain) wrote…

pl_cachemb

pl_cache_mb


posting/lists.go, line 168 at r1 (raw file):

Previously, manishrjain (Manish R Jain) wrote…

This would be tricky because we'll have many caches -- also from Badger.

Perhaps move to V(2) for now. @jarifibrahim should move all of these metrics out to prometheus metrics.


posting/lists.go, line 165 at r2 (raw file):

	go func() {
		m := lCache.Metrics
		ticker := time.NewTicker(60 * time.Second)

5 * time.Minute


posting/mvcc.go, line 268 at r1 (raw file):

Previously, manishrjain (Manish R Jain) wrote…

Maybe not Del, instead do Set.

Maybe add a note that we should perhaps try using Set with nil value to deal with jepsen failures.


posting/mvcc.go, line 282 at r2 (raw file):

func WaitForCache() {
	lCache.Wait()

Maybe comment this out for now. Add a note to investigate and better understand why Jepsen fails with cache on.

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, 5 unresolved discussions (waiting on @balajijinnah, @jarifibrahim, @manishrjain, and @vvbalaji-dgraph)


dgraph/cmd/alpha/run.go, line 203 at r1 (raw file):

Previously, manishrjain (Manish R Jain) wrote…

pl_cache_mb

Done.


posting/lists.go, line 168 at r1 (raw file):

Previously, manishrjain (Manish R Jain) wrote…

Perhaps move to V(2) for now. @jarifibrahim should move all of these metrics out to prometheus metrics.

Done.


posting/lists.go, line 165 at r2 (raw file):

Previously, manishrjain (Manish R Jain) wrote…

5 * time.Minute

Done.


posting/mvcc.go, line 268 at r1 (raw file):

Previously, manishrjain (Manish R Jain) wrote…

Maybe add a note that we should perhaps try using Set with nil value to deal with jepsen failures.

Done.


posting/mvcc.go, line 282 at r2 (raw file):

Previously, manishrjain (Manish R Jain) wrote…

Maybe comment this out for now. Add a note to investigate and better understand why Jepsen fails with cache on.

Done.

@martinmr martinmr merged commit ccfa6a4 into master Aug 24, 2020
@martinmr martinmr deleted the martinmr/cache-with-flag branch August 24, 2020 13:54
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