sql,jobs,ccl: add ability to specify a REASON string with PAUSE JOB

[rhu713]

Add ability to specify a REASON string with PAUSE JOB, persist this string in the job's payload. See individual commits.

[cockroach-teamcity]

This change is 

[sajjadrizvi]

Changes in jobs package look good to me.

Reviewable status: complete! 0 of 0 LGTMs obtained

[ajwerner]

You've got some build issues. Otherwise, change looks good to me mod the call for the defensive check.

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @rhu713)

---

pkg/jobs/jobs_test.go, line 3181 at r3 (raw file):

Quoted 8 lines of code…

		var payloadBytes []byte
		var payload jobspb.Payload
		var status string
		tdb.QueryRow(t, fmt.Sprintf("SELECT status, payload FROM system.jobs where id = %d", jobID)).Scan(
			&status, &payloadBytes)
		require.Equal(t, "paused", status)
		require.NoError(t, protoutil.Unmarshal(payloadBytes, &payload))
		require.Equal(t, "for testing", payload.PauseReason)

nit: make yourself some closures for this. Also pass the ID as a parameter to the query:

getStatusAndPayload := func(t *testing.T, id jobspb.ID) (status string, payload jobspb.Payload) {
    var payloadBytes []byte
    tdb.QueryRow(
        t, "SELECT status, payload FROM system.jobs where id = $1", jobID,
    ).Scan(&status, &payloadBytes)
    require.NoError(t, protoutil.Unmarshal(payloadBytes, &payload))
    return status, payload
}
checkStatusAndPauseReason := func(t *testing.T, id jobspb.ID, expStatus, expPauseReason string). {
    status, payload := getStatusAndPayload(t, id)
    require.Equal(t, expStatus, status, "status")
    require.Equal(t, expPauseReason, payload.PauseReason, "pause reason")
}

Then these will look like:

	{
		// Next, pause the job with a reason. Wait for pause and make sure the pause reason is set.
		require.NoError(t, registry.PauseRequested(ctx, nil, jobID, "for testing"))
		tdb.CheckQueryResultsRetry(t, q, [][]string{{"paused"}})
                checkStatusAndPauseReason("paused", "for testing")
        }

---

pkg/jobs/jobs_test.go, line 3197 at r3 (raw file):

		require.NoError(t, protoutil.Unmarshal(payloadBytes, &payload))
		require.Equal(t, "for testing", payload.PauseReason)
		fmt.Println(payload)

remove

---

pkg/sql/control_jobs.go, line 65 at r3 (raw file):

		}
	}

How do you feel about adding an assertion that reason is empty unless the desiredStatus is paused? I know the grammar doesn't allow it but we've been hurt in the past when we change the grammar and then don't realize that assumptions have been violated.

[rhu713]

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

---

pkg/jobs/jobs_test.go, line 3181 at r3 (raw file):

Previously, ajwerner wrote…

		var payloadBytes []byte
		var payload jobspb.Payload
		var status string
		tdb.QueryRow(t, fmt.Sprintf("SELECT status, payload FROM system.jobs where id = %d", jobID)).Scan(
			&status, &payloadBytes)
		require.Equal(t, "paused", status)
		require.NoError(t, protoutil.Unmarshal(payloadBytes, &payload))
		require.Equal(t, "for testing", payload.PauseReason)

nit: make yourself some closures for this. Also pass the ID as a parameter to the query:

getStatusAndPayload := func(t *testing.T, id jobspb.ID) (status string, payload jobspb.Payload) {
    var payloadBytes []byte
    tdb.QueryRow(
        t, "SELECT status, payload FROM system.jobs where id = $1", jobID,
    ).Scan(&status, &payloadBytes)
    require.NoError(t, protoutil.Unmarshal(payloadBytes, &payload))
    return status, payload
}
checkStatusAndPauseReason := func(t *testing.T, id jobspb.ID, expStatus, expPauseReason string). {
    status, payload := getStatusAndPayload(t, id)
    require.Equal(t, expStatus, status, "status")
    require.Equal(t, expPauseReason, payload.PauseReason, "pause reason")
}

Then these will look like:

	{
		// Next, pause the job with a reason. Wait for pause and make sure the pause reason is set.
		require.NoError(t, registry.PauseRequested(ctx, nil, jobID, "for testing"))
		tdb.CheckQueryResultsRetry(t, q, [][]string{{"paused"}})
                checkStatusAndPauseReason("paused", "for testing")
        }

Done, thanks!

---

pkg/jobs/jobs_test.go, line 3197 at r3 (raw file):

Previously, ajwerner wrote…

remove

Done

---

pkg/sql/control_jobs.go, line 65 at r3 (raw file):

Previously, ajwerner wrote…

How do you feel about adding an assertion that reason is empty unless the desiredStatus is paused? I know the grammar doesn't allow it but we've been hurt in the past when we change the grammar and then don't realize that assumptions have been violated.

Okay, added this this defensive check. Let me know what you think.

[ajwerner]

Reviewed 1 of 15 files at r8, 2 of 6 files at r9.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @dt)

[rhu713]

bors r+

[craig]

Build succeeded:

• GitHub CI (Cockroach)

[mgartner]

Sorry for the late comment, but I'm curious what the motivation was to include the = token? Was PAUSE JOB a_expr WITH REASON string_or_placeholder an ambiguous syntax?