workload/ycsb: support workload D, E and fix key selection for insertion and reading

[jeffrey-xiao]

This PR does the following items:

1. Support workload D (95% reads skewed to newer keys, 5% writes) by adding a latest generator
2. Support workload E (95% scans, 5% writes) by implementing scans
3. Fix key selection for insertion by having a separate index for next row to insert rather than using the iMaxHead in the zipfian generator
4. Fix key selection for reading by having a separate counter for number of rows instead of using the number of initial rows. I didn't fully understand the method proposed in the ycsb paper in section 5.3, so if this isn't the best way, I'm open to changes.
5. Improve key selection by hashing before modding. This follows the logic proposed in the ycsb paper. Originally we were hashing after applying a modulo, but that always causes the first inserted key to be the hotspot rather than distribute the hotspot around.
6. Change API for generators to be more consistent. All generators now take in a lower bound and upper bound and generate an inclusive range between the lower and upper bound (I.E. [lower, upper]).

[cockroach-teamcity]

This change is 

[ajwerner]

Very cool! Have you tried running them on a roachprod cluster? Consider adding these workloads to the set of ycsb roachtests and then running them and seeing what you see. Both of these are useful and realistic workloads.

Reviewed 1 of 1 files at r1, 2 of 2 files at r2, 2 of 2 files at r3.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @jeffrey-xiao and @nvanbenschoten)

---

pkg/workload/ycsb/ycsb.go, line 338 at r1 (raw file):

	updateStmts []*gosql.Stmt

	// The next row index to insert.

Seems like you never want these to be nil pointers when you interact with them below. How about avoiding the allocations above and just taking the address of the field for use with the atomics?

---

pkg/workload/ycsb/ycsb.go, line 126 at r3 (raw file):

		g.flags.StringVar(&g.workload, `workload`, `B`, `Workload type. Choose from A-F.`)
		g.flags.StringVar(&g.requestDistribution, `request-distribution`, `zipfian`, `Distribution for request key generation [zipfian, uniform, latest].`)
		g.flags.StringVar(&g.scanLengthDistribution, `scan-length-distribution`, `uniform`, `Distribution for scan length generation [zipfian, uniform].`)

nit: can you augment the help text for these 3 flags to mention that this is for workload E.

---

pkg/workload/ycsb/ycsb.go, line 510 at r3 (raw file):

	defer res.Close()
	for res.Next() {

nit: no need for this blank line. I guess if you feel like it's visually important you can add a comment inside the loop.

[jeffrey-xiao]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ajwerner and @nvanbenschoten)

---

pkg/workload/ycsb/ycsb.go, line 338 at r1 (raw file):

Previously, ajwerner wrote…

Seems like you never want these to be nil pointers when you interact with them below. How about avoiding the allocations above and just taking the address of the field for use with the atomics?

The row indexes and counters are shared between workers, so they have to be allocated.

[ajwerner]

Reviewed 1 of 1 files at r4, 1 of 1 files at r5.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @nvanbenschoten)

[nvanbenschoten]

Very nice!

Reviewed 2 of 2 files at r2, 2 of 2 files at r3, 1 of 1 files at r4, 1 of 1 files at r5, 4 of 4 files at r6, 2 of 2 files at r7, 1 of 1 files at r8.
Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @jeffrey-xiao)

---

pkg/workload/ycsb/latest_generator.go, line 24 at r2 (raw file):

)

// LatestGenerator is a random number generator that generates draws from a

The official YCSB benchmark seems to use a skewed latest generator for workload D. Why did you decide to go with a uniform distribution here?

EDIT: never mind, looks like you realized this already. Want to call this SkewedLatestGenerator to mirror their implementation?

---

pkg/workload/ycsb/uniform_generator.go, line 62 at r3 (raw file):

}

// Uint64 returns a random Uint64 between iMin and iMax, drawn from a uniform

How does this ensure that the value is above iMin?

---

pkg/workload/ycsb/ycsb.go, line 406 at r1 (raw file):

// See YCSB paper section 5.3 for a complete description of how keys are chosen.
func (yw *ycsbWorker) nextReadKey() string {
	rowCount := atomic.LoadUint64(yw.rowCount)

Do we ever modify rowCount? Do we need this atomic load?

---

pkg/workload/ycsb/ycsb.go, line 144 at r2 (raw file):

	return workload.Hooks{
		Validate: func() error {
			switch g.workload {

Do you think we should be defaulting the request distribution differently depending on the workload?

---

pkg/workload/ycsb/ycsb.go, line 307 at r3 (raw file):

		return workload.QueryLoad{}, errors.Errorf("Unknown request distribution: %s", g.requestDistribution)
	}
	if err != nil {

Nice catch!

[jeffrey-xiao]

Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @nvanbenschoten)

---

pkg/workload/ycsb/uniform_generator.go, line 62 at r3 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

How does this ensure that the value is above iMin?

Oops, forgot to make this change.

---

pkg/workload/ycsb/ycsb.go, line 406 at r1 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

Do we ever modify rowCount? Do we need this atomic load?

Also forgot to increment this when iMax gets incremented.

---

pkg/workload/ycsb/ycsb.go, line 144 at r2 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

Do you think we should be defaulting the request distribution differently depending on the workload?

I think that's a reasonable change.

[petermattis]

Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @jeffrey-xiao and @nvanbenschoten)

---

pkg/workload/ycsb/uniform_generator.go, line 67 at r12 (raw file):

	z.mu.Lock()
	defer z.mu.Unlock()
	return rand.Uint64()%(z.mu.iMax-z.iMin+1) + z.iMin

This should be using z.mu.r.Uint64(), not the global rand.Uint64().

[petermattis]

Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @jeffrey-xiao and @nvanbenschoten)

---

pkg/workload/ycsb/uniform_generator.go, line 67 at r12 (raw file):

Previously, petermattis (Peter Mattis) wrote…

This should be using z.mu.r.Uint64(), not the global rand.Uint64().

Also, using mod to get integers within a range is slightly biased. This code should probably be using math/rand.Rand.Int63n. I realize this wasn't changed in this PR, but as long as you're in here...

[petermattis]

Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @jeffrey-xiao and @nvanbenschoten)

---

pkg/workload/ycsb/ycsb.go, line 409 at r12 (raw file):

	// distribution and modding the samples. To properly support INSERTS, we need
	// to maintain a separate rownum counter.
	rownum := yw.randGen.IMaxHead()

This was the only call to IMaxHead. I think we can remove that method now, which was a bit of an oddity in the random generators.

---

pkg/workload/ycsb/ycsb.go, line 438 at r12 (raw file):

func (yw *ycsbWorker) nextReadKey() string {
	rowCount := atomic.LoadUint64(yw.rowCount)
	rowIndex := yw.hashKey(yw.requestGen.Uint64()) % rowCount

buildKeyName also calls hashKey, so this will cause us to call hashKey twice for each read key. Is that expected?

---

pkg/workload/ycsb/ycsb.go, line 472 at r12 (raw file):

			return err
		}
		atomic.AddUint64(yw.rowCount, 1)

Can you confirm that this is what the official YCSB implementation does? I ask because it is still possible to generate a read key which hasn't been inserted yet due to concurrency in the inserts.

[jeffrey-xiao]

Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @nvanbenschoten and @petermattis)

---

pkg/workload/ycsb/uniform_generator.go, line 67 at r12 (raw file):

Previously, petermattis (Peter Mattis) wrote…

Also, using mod to get integers within a range is slightly biased. This code should probably be using math/rand.Rand.Int63n. I realize this wasn't changed in this PR, but as long as you're in here...

Done. Unfortunate that there is no Uint64n.

---

pkg/workload/ycsb/ycsb.go, line 409 at r12 (raw file):

Previously, petermattis (Peter Mattis) wrote…

This was the only call to IMaxHead. I think we can remove that method now, which was a bit of an oddity in the random generators.

Done.

---

pkg/workload/ycsb/ycsb.go, line 438 at r12 (raw file):

Previously, petermattis (Peter Mattis) wrote…

buildKeyName also calls hashKey, so this will cause us to call hashKey twice for each read key. Is that expected?

The issue with the previous implementation is that it gets hashed after the mod which always causes the hotspot to be the first key. The yscb paper suggests to hash then mod. Hashing twice for each read key is also consistent with the official ycsb implementation. When the request distribution is zipfian, the official yscb implementation uses a ScrambledZipfianGenerator which hashes the output of a ZipfianGenerator. Then they hash the generated key num to obtain the key name.

---

pkg/workload/ycsb/ycsb.go, line 472 at r12 (raw file):

Previously, petermattis (Peter Mattis) wrote…

Can you confirm that this is what the official YCSB implementation does? I ask because it is still possible to generate a read key which hasn't been inserted yet due to concurrency in the inserts.

Thanks for the catch. The official implementation uses an AcknowledgedCounterGenerator which I've now added.

[petermattis]

Reviewable status: complete! 1 of 0 LGTMs obtained (and 1 stale) (waiting on @jeffrey-xiao and @nvanbenschoten)

---

pkg/workload/ycsb/acknowledged_counter.go, line 45 at r16 (raw file):

	c := &AcknowledgedCounter{}
	c.mu.count = initialCount
	c.mu.window = make([]bool, windowSize)

I believe this will use windowSize bytes. You could reduce that by a factor of 8 by using a bitmap. Not sure if that is worth it as we're only talking about 1MB of memory and the current code is clear and concise.

---

pkg/workload/ycsb/uniform_generator.go, line 67 at r12 (raw file):

Previously, jeffrey-xiao (Jeffrey Xiao) wrote…

Done. Unfortunate that there is no Uint64n.

Someday (not today) we may upgrade to golang.org/x/exp/rand which does have Uint64n.

[jeffrey-xiao]

bors r+

[craig]

Build succeeded

• GitHub CI (Cockroach)