cockroach icon indicating copy to clipboard operation
cockroach copied to clipboard

roachtest: make validation of workload voluntary

Open dt opened this issue 1 year ago • 4 comments

Similar to https://github.com/cockroachdb/cockroach/pull/115947 but now with also opting in many tests that were easily identified by grep as needing the old binary into validating it.

dt avatar Feb 22 '24 16:02 dt

This change is Reviewable

cockroach-teamcity avatar Feb 22 '24 16:02 cockroach-teamcity

I'll leave the discussion on if we should add this up to the others since I have no strong opinion/limited insight.

But if we do go ahead with this change it'd be nice to force any tests that use t.DeprecatedWorkload() to also set RequiresDeprecatedWorkload: true. With how it is currently I could see a lot of people copy and pasting the c.Put() without also copying the RequiresDeprecatedWorkload: true

Failing at compile time or before the test starts would be great, but not sure how feasible that is. At the minimum something like:

func (t *testImpl) DeprecatedWorkload() string {
	if !t.spec.RequiresDeprecatedWorkload {
		t.Fatalf("helpful error about how RequiresDeprecatedWorkload should be set")
	}
	return t.deprecatedWorkload
}

might be good?

DarrylWong avatar Feb 22 '24 19:02 DarrylWong

Failing at compile time or before the test starts would be great

This seems like way more work than it would be worth for what is, to be clear, a deprecated field. I'm not even 100% convinced the fuss of opt-in validation is worth it for a deprecated feature; I only added it here if it helped folks be more okay with remove the requirement from the large majority of tests that are having it inflicted on them for no reason. I'm not super motivated to make it more complex / checked at runtime / etc to better handle new tests, since new tests shouldn't be using something marked as "deprecated" anyway.

dt avatar Feb 22 '24 19:02 dt

This seems like way more work than it would be worth for what is, to be clear, a deprecated field.

It's not really a deprecated field despite having that moniker. The issue is that we have a number of workloads which are not part of the cockroach binary, for the simple reason of trying to contain its size. I don't see a quick resolution here, so the workload will likely have to remain undeprecated.

srosenberg avatar Feb 23 '24 05:02 srosenberg