batch hot paths for a very short duration #1618

ozkatz · 2021-03-12T19:12:30Z

Looking at the access pattern for critical path operations, we mostly call PostgreSQL with the same exact queries many times.

For GetObject, StatObject, ListObjects (which probably make up the majority of data lake calls), we ALWAYS start by doing the same set of roundtrips to PG:

Get the repository (to extract the storage namespace)
Resolve the ref (i.e. figure out if this is a commit/commit prefix/branch/tag)
Resolve the underlying commit ID (if a branch, prefix or tag)

Our access pattern is such that many requests at a given point in time are extremely likely to not only share the same repository details, but also the same branch/commit/tag, as big data systems tend to be bursty in nature.

Since caching is not an option since it sacrifices consistency (in the sense that reading after a successful write returns - might return a stale value), instead of keeping the result around for a while, we can keep the requests around for a while.

This is what this PR does: for a given type of request (i.e. to a specific branch/repo/tag, etc), wait a couple of milliseconds: if other identical requests arive in that time, do a single roundtrip and return the results once for all those requests.

Testing this on the same environment used for the sizing guide (2 x c5ad.xlarge AWS instances), I now get the following results:

lakectl abuse random-read on a commit ID: throughput goes up from 8-10k requests/second to 45k requests/second
Amount of DB queries drops from ~20k/s to less than 1k/s.

I'm OK with not accepting this due to it being a premature optimization (it is!), but I feel the added complexity is relatively small and the gain is pretty big (if only to show better numbers per core as possible).

nopcoder · 2021-03-12T19:25:12Z

Think we can we can simplify it a bit more while using singleflight package + wrapping the function call with the same delay before calling the actual function that fetches the data.

ozkatz · 2021-03-12T19:56:43Z

Think we can we can simplify it a bit more while using singleflight package + wrapping the function call with the same delay before calling the actual function that fetches the data.

@nopcoder sounds nice! Feel free to give that a go 🙂

nopcoder · 2021-03-12T21:53:15Z

Think we can we can simplify it a bit more while using singleflight package + wrapping the function call with the same delay before calling the actual function that fetches the data.

@nopcoder sounds nice! Feel free to give that a go 🙂

#1620

Need to set up a test env for testing the above.

ozkatz · 2021-03-13T15:41:18Z

Think we can we can simplify it a bit more while using singleflight package + wrapping the function call with the same delay before calling the actual function that fetches the data.

@nopcoder sounds nice! Feel free to give that a go slightly_smiling_face

#1620

Need to set up a test env for testing the above.

Used the same env to test your branch - behaves just the same (~45k requests/second). I agree your implementation is simpler.

nopcoder · 2021-03-14T09:36:45Z

pkg/batch/executor.go

+		keys:     make(map[string][]*request),
+		logger:   logger,
+	}
+	go e.Run() // TODO(ozkatz): should probably be managed by the user (also, allow stopping it)


You can move this one into Run() with defer close, so we will not require to add Stop() and/or Close() methods to handle this resource.

pkg/graveler/ref/manager.go

nopcoder · 2021-03-14T09:40:22Z

pkg/batch/executor.go

+			// see if we have it scheduled already
+			if _, exists := e.keys[req.key]; !exists {
+				// this is a new key, let's fire a timer for it
+				go func(req *request) {


Add WaitGroup that will count ongoing goroutines.

Add Close() method to Executor to wait for the wait group done.

Close() should also train execs and call the responseCallback(s)

nopcoder · 2021-03-14T09:45:09Z

pkg/batch/executor.go

+			// let's take all callbacks
+			waiters := e.keys[execKey]
+			delete(e.keys, execKey)
+			go func(key string) {


pass waiters as you pass the key - just to be symmetric

Or don't pass both and pin instead..

nopcoder · 2021-03-14T09:49:24Z

pkg/batch/executor.go

+			delete(e.keys, execKey)
+			go func(key string) {
+				// execute and call all mapped callbacks
+				v, err := waiters[0].fn()


Suggestion: will probably will like to capture this one inside a func with recover that will return an error

codecov-io · 2021-03-14T11:07:37Z

Codecov Report

Merging #1618 (93337f1) into master (b833114) will increase coverage by 0.16%.
The diff coverage is 79.61%.

@@            Coverage Diff             @@
##           master    #1618      +/-   ##
==========================================
+ Coverage   39.25%   39.41%   +0.16%     
==========================================
  Files         167      168       +1     
  Lines       13563    13621      +58     
==========================================
+ Hits         5324     5369      +45     
- Misses       7474     7487      +13     
  Partials      765      765

Impacted Files	Coverage Δ
pkg/catalog/catalog.go	`18.80% <0.00%> (-0.27%)`	⬇️
pkg/batch/executor.go	`88.88% <88.88%> (ø)`
pkg/graveler/ref/manager.go	`71.92% <89.28%> (+1.92%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update dc0fdf0...93337f1. Read the comment docs.

ozkatz · 2021-03-14T11:10:34Z

@itaiad200 please see the tests I added: I attempted to prove this method does not violate read-after-write consistency

itaiad200 · 2021-03-14T11:37:03Z

pkg/batch/executor.go

+			// let's take all callbacks
+			waiters := e.keys[execKey]
+			delete(e.keys, execKey)
+			go func(key string) {


Or don't pass both and pin instead..

pkg/catalog/catalog.go

pkg/graveler/ref/manager.go

pkg/batch/executor.go

arielshaqed

Thanks!

Most requested is the change to the test, to ensure reader1 actually starts waiting after writer1 writes.

pkg/batch/executor.go

arielshaqed · 2021-03-14T13:33:23Z

pkg/batch/executor.go

+	responseCallback chan *response
+}
+
+type Executor struct {


An Executor is a Batcher, which is a somewhat odd usage of the interface name

I'm bad at naming, I'll admit to that. Suggestions are welcome :)

pkg/batch/executor.go

pkg/batch/executor_test.go

arielshaqed · 2021-03-14T14:08:17Z

pkg/batch/executor_test.go

+	delayFn := func(dur time.Duration) {
+		delaysDone := atomic.AddInt32(&delays, 1)
+		if delaysDone == 1 {
+			close(waitWrite)


Note that the write can occur before https://github.com/treeverse/lakeFS/pull/1618/files#diff-c9e7aae146c0798d32ade9be6fa5013612246e323b7a2796dbcdc83a0151c607R82 ever happens (because the scheduler is evil). I think you may need to wait on another channel here, that writer1 will close after it does write.

pkg/catalog/catalog.go

pkg/graveler/ref/manager.go

batch hot paths for a very short duration

c9b07b3

ozkatz added the improvement label Mar 12, 2021

ozkatz requested review from nopcoder, johnnyaug, arielshaqed, itaiad200 and guy-har March 12, 2021 19:12

ozkatz self-assigned this Mar 12, 2021

nopcoder requested changes Mar 14, 2021

View reviewed changes

added tests and a controlled runner that can be stopped

1408ce5

removed blank lines

93337f1

ozkatz requested a review from nopcoder March 14, 2021 11:10

ozkatz marked this pull request as ready for review March 14, 2021 12:02

nopcoder approved these changes Mar 14, 2021

View reviewed changes

itaiad200 reviewed Mar 14, 2021

View reviewed changes

added a small channel buffer and document max batch time

e8f76ea

arielshaqed reviewed Mar 14, 2021

View reviewed changes

better parameter naming, run consistency test multiple times in parallel

1cd1ecb

itaiad200 approved these changes Mar 14, 2021

View reviewed changes

ozkatz merged commit c68123e into master Mar 14, 2021

ozkatz deleted the feature/delay branch March 14, 2021 15:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

batch hot paths for a very short duration #1618

batch hot paths for a very short duration #1618

ozkatz commented Mar 12, 2021

nopcoder commented Mar 12, 2021

ozkatz commented Mar 12, 2021

nopcoder commented Mar 12, 2021

ozkatz commented Mar 13, 2021

nopcoder Mar 14, 2021

nopcoder Mar 14, 2021

nopcoder Mar 14, 2021

itaiad200 Mar 14, 2021

nopcoder Mar 14, 2021

codecov-io commented Mar 14, 2021 •

edited

Loading

ozkatz commented Mar 14, 2021

itaiad200 Mar 14, 2021

arielshaqed left a comment

arielshaqed Mar 14, 2021

ozkatz Mar 14, 2021

arielshaqed Mar 14, 2021

batch hot paths for a very short duration #1618

batch hot paths for a very short duration #1618

Conversation

ozkatz commented Mar 12, 2021

nopcoder commented Mar 12, 2021

ozkatz commented Mar 12, 2021

nopcoder commented Mar 12, 2021

ozkatz commented Mar 13, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-io commented Mar 14, 2021 • edited Loading

Codecov Report

ozkatz commented Mar 14, 2021

Choose a reason for hiding this comment

arielshaqed left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-io commented Mar 14, 2021 •

edited

Loading