cdc: Add ledger workload option to CDC roachtests

[mrtracy]

Adds a new workload option to the CDC tests "ledger", which runs the
ledger workload instead of the TPCC workload.

To accomodate this, significantly refactor the CDC test (already
confined to a single file, not a far-reaching refactor). Notes on this:

• Breaks down the large test function into several logical components -
  a workload, a verifier, a manager for kafka - and encapsulate those in
  simple objects with methods. Makes the top-level flow much easier to
  read.
• Move all verification parameters (e.g. maximum allowed latency) to be
  explicitly defined for each test, rather than being inferred within the
  test from other parameters such as warehouse count.

Release note: None

[cockroach-teamcity]

This change is 

[cockroach-teamcity]

This change is 

[danhhz]

I wish I'd knows you were planning a refactor this big. This conflicts with some changes I have in local PRs, which I'll now have to resolve and keep rebased on top of this while you iterate. Perhaps worse is that this set of tests has been tricky to get right and now is a bad time to have to start debugging any issues introduced by a refactor. It's definitely more clear now, so I guess we merge it, but let's please stop refactoring this file for a while

Go ahead and run these to make sure they still pass and manually verify that everything looks sane

Reviewable status: complete! 0 of 0 LGTMs obtained

---

pkg/cmd/roachtest/cdc.go, line 33 at r1 (raw file):

const (
	tpccWorkloadType workloadType = iota

no iota please. explicitly assigning the ints is basically always clearer

even better is to make workloadType a string and make these "tpcc" and "ledger" so they're useful when printed

---

pkg/cmd/roachtest/cdc.go, line 59 at r1 (raw file):

	c.Start(ctx, t, crdbNodes, startArgs(`--args=--vmodule=changefeed=2,poller=2`))

	// Additional arcane test setup, just to make things run more smoothly.

see below comment. this is arguably already lying

---

pkg/cmd/roachtest/cdc.go, line 70 at r1 (raw file):

	}()

	// Install and start Kafka.

In general, I find these sorts of narrative comments more harmful than helpful. This isn't adding anything to the below, but at best it has to be maintained in future changes and worse it could rot and become a lie

---

pkg/cmd/roachtest/cdc.go, line 215 at r1 (raw file):

		Name:       "cdc/ledger/nodes=3/init=false",
		MinVersion: "2.1.0",
		Nodes:      nodes(4, cpu(16)),

should be cpu(20) right?

---

pkg/cmd/roachtest/cdc.go, line 221 at r1 (raw file):

				workloadType:             ledgerWorkloadType,
				workloadDuration:         "30m",
				initialScan:              true,

this disagrees with init=false in the name

---

pkg/cmd/roachtest/cdc.go, line 318 at r1 (raw file):

func (lw *ledgerWorkload) run(ctx context.Context, c *cluster, workloadDuration string) {
	c.Run(ctx, lw.workloadNodes, fmt.Sprintf(
		`./workload run ledger --mix=balance=50,withdrawal=25,deposit=12,reversal=0 {pgurl%s} --workloadDuration=%s`,

#28642 says withdraw=50,deposit=50, right? it also says 575 txn/s, which we might have to add support for in the workload, if it doesn't support it already

[mrtracy]

I have already manually verified that the roachtests are still working correctly.

Reviewable status: complete! 0 of 0 LGTMs obtained

---

pkg/cmd/roachtest/cdc.go, line 33 at r1 (raw file):

Previously, danhhz (Daniel Harrison) wrote…

no iota please. explicitly assigning the ints is basically always clearer

even better is to make workloadType a string and make these "tpcc" and "ledger" so they're useful when printed

done.

---

pkg/cmd/roachtest/cdc.go, line 70 at r1 (raw file):

Previously, danhhz (Daniel Harrison) wrote…

In general, I find these sorts of narrative comments more harmful than helpful. This isn't adding anything to the below, but at best it has to be maintained in future changes and worse it could rot and become a lie

I will remove them. I will say, for me they serve to break up the code into clear steps. Without them, visually the code seems to run together and be harder to parse.

I do understand the sentiment about these comments getting out of date.

---

pkg/cmd/roachtest/cdc.go, line 215 at r1 (raw file):

Previously, danhhz (Daniel Harrison) wrote…

should be cpu(20) right?

Apparently this is not available from GCE? I got an error, we're going to have to figure out how to get this to the right number.

---

pkg/cmd/roachtest/cdc.go, line 221 at r1 (raw file):

Previously, danhhz (Daniel Harrison) wrote…

this disagrees with init=false in the name

Done.

---

pkg/cmd/roachtest/cdc.go, line 318 at r1 (raw file):

Previously, danhhz (Daniel Harrison) wrote…

#28642 says withdraw=50,deposit=50, right? it also says 575 txn/s, which we might have to add support for in the workload, if it doesn't support it already

is balance supposed to be 0?

As for 575txn/s, I need to add a new verification step (and possibly workload options) for that to make sure the test is actually hitting that target. I held off on that because this was already a big change.

[danhhz]

I have already manually verified that the roachtests are still working correctly.

Gotcha. The PR description is out of date then.

Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale)

---

pkg/cmd/roachtest/cdc.go, line 70 at r1 (raw file):

Previously, mrtracy (Matt Tracy) wrote…

I will remove them. I will say, for me they serve to break up the code into clear steps. Without them, visually the code seems to run together and be harder to parse.

I do understand the sentiment about these comments getting out of date.

Yeah, it's always a balance. You've added some of these in other places in recent PRs and I thought they were a net win. These ones I felt went a bit the other way

---

pkg/cmd/roachtest/cdc.go, line 215 at r1 (raw file):

Previously, mrtracy (Matt Tracy) wrote…

Apparently this is not available from GCE? I got an error, we're going to have to figure out how to get this to the right number.

ah, makes sense. leave a TODO please

---

pkg/cmd/roachtest/cdc.go, line 318 at r1 (raw file):

is balance supposed to be 0?

I forwarded you the email that I got those numbers from. Please do double check what I came up with.

As for 575txn/s, I need to add a new verification step (and possibly workload options) for that to make sure the test is actually hitting that target. I held off on that because this was already a big change.

leave a TODO please

[mrtracy]

Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale)

---

pkg/cmd/roachtest/cdc.go, line 215 at r1 (raw file):

Previously, danhhz (Daniel Harrison) wrote…

ah, makes sense. leave a TODO please

Done.

---

pkg/cmd/roachtest/cdc.go, line 318 at r1 (raw file):

Previously, danhhz (Daniel Harrison) wrote…

is balance supposed to be 0?

I forwarded you the email that I got those numbers from. Please do double check what I came up with.

As for 575txn/s, I need to add a new verification step (and possibly workload options) for that to make sure the test is actually hitting that target. I held off on that because this was already a big change.

leave a TODO please

Done, and I think 0/50/50 is fine for now. We can add additional tests with different parameters if necessary.

[mrtracy]

bors r=danhhz

[mrtracy]

bors r=danhhz

[craig]

Build succeeded

• GitHub CI (Cockroach)

[craig]

Build succeeded

• GitHub CI (Cockroach)

[craig]

Not awaiting review

[craig]

Not awaiting review