From d05211a43492aeed60e3669788cd4b6a17790ca5 Mon Sep 17 00:00:00 2001 From: Raphael 'kena' Poss Date: Sat, 8 Apr 2023 11:45:11 +0200 Subject: [PATCH 1/5] jobs: fix an error message Error messages are lowercase. Release note: None --- pkg/jobs/delegate_control_test.go | 2 +- pkg/sql/delegate/job_control.go | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/pkg/jobs/delegate_control_test.go b/pkg/jobs/delegate_control_test.go index b06cff11a92a..f69aefe91f6a 100644 --- a/pkg/jobs/delegate_control_test.go +++ b/pkg/jobs/delegate_control_test.go @@ -359,7 +359,7 @@ func TestJobControlByType(t *testing.T) { sessiondata.RootUserSessionDataOverride, invalidTypeQuery, ) - require.Error(t, err) + require.Error(t, err, "unsupported job type") }) // To test the commands on valid job types, one job of every type in every state will be created diff --git a/pkg/sql/delegate/job_control.go b/pkg/sql/delegate/job_control.go index d5aa83fdb7ab..772f5fa94016 100644 --- a/pkg/sql/delegate/job_control.go +++ b/pkg/sql/delegate/job_control.go @@ -78,7 +78,7 @@ AND jobs.status IN (%s) AND jobs.created_by_id IN (%s)`, if stmt.Type != "" { if _, ok := protobufNameForType[stmt.Type]; !ok { - return nil, errors.New("Unsupported job type") + return nil, errors.New("unsupported job type") } queryStrFormat := `%s JOBS ( SELECT id From 306ae9068ff159c8c88a179008d31e5d1090fa2c Mon Sep 17 00:00:00 2001 From: Raphael 'kena' Poss Date: Sat, 8 Apr 2023 11:46:28 +0200 Subject: [PATCH 2/5] jobs: fix comments in test Release note: None --- pkg/jobs/delegate_control_test.go | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/pkg/jobs/delegate_control_test.go b/pkg/jobs/delegate_control_test.go index f69aefe91f6a..09ec82ea4a71 100644 --- a/pkg/jobs/delegate_control_test.go +++ b/pkg/jobs/delegate_control_test.go @@ -362,7 +362,7 @@ func TestJobControlByType(t *testing.T) { require.Error(t, err, "unsupported job type") }) - // To test the commands on valid job types, one job of every type in every state will be created + // To test the commands on valid job types, one job of every type in every state will be created. var allJobTypes = []jobspb.Type{jobspb.TypeChangefeed, jobspb.TypeImport, jobspb.TypeBackup, jobspb.TypeRestore} var jobspbTypeToString = map[jobspb.Type]string{ jobspb.TypeChangefeed: "CHANGEFEED", @@ -374,7 +374,7 @@ func TestJobControlByType(t *testing.T) { var allJobStates = []Status{StatusPending, StatusRunning, StatusPaused, StatusFailed, StatusReverting, StatusSucceeded, StatusCanceled, StatusCancelRequested, StatusPauseRequested} - // This is required to make the jobs of each type controllable + // Make the jobs of each type controllable. for _, jobType := range allJobTypes { RegisterConstructor(jobType, func(job *Job, _ *cluster.Settings) Resumer { return FakeResumer{ @@ -400,7 +400,7 @@ func TestJobControlByType(t *testing.T) { t.Run(commandQuery, func(t *testing.T) { var jobIDStrings []string - // Make multiple jobs of every permutation of job type and job state + // Make multiple jobs of every permutation of job type and job state. const numJobsPerStatus = 3 for _, jobInfo := range []struct { jobDetails jobspb.Details @@ -431,7 +431,7 @@ func TestJobControlByType(t *testing.T) { jobIdsClause := fmt.Sprint(strings.Join(jobIDStrings, ", ")) - // Execute the command and verify its executed on the expected number of rows + // Execute the command and verify it is executed on the expected number of rows. numEffected, err := th.cfg.DB.Executor().ExecEx( context.Background(), "test-num-effected", @@ -441,16 +441,16 @@ func TestJobControlByType(t *testing.T) { ) require.NoError(t, err) - // Jobs in the starting state should be affected + // Jobs in the starting state should be affected. numExpectedJobsAffected := numJobsPerStatus * len(tc.startingStates) require.Equal(t, numExpectedJobsAffected, numEffected) - // Both the affected jobs + the jobs originally in the target state should be in that state + // Both the affected jobs + the jobs originally in the target state should be in that state. numExpectedJobsWithEndState := numExpectedJobsAffected + numJobsPerStatus // By verifying that the correct number of jobs are in the expected end state and // the expected number of jobs were affected by the command, we guarantee that - // only the expected jobs have changed + // only the expected jobs have changed. var numJobs = 0 th.sqlDB.QueryRow( t, From 5180bb9374cc862d13ebb685b42342fb9d99ab8a Mon Sep 17 00:00:00 2001 From: Raphael 'kena' Poss Date: Sat, 8 Apr 2023 11:48:51 +0200 Subject: [PATCH 3/5] jobs: skip TestJobControlByType in short tests It's a very long test. Release note: None --- pkg/jobs/delegate_control_test.go | 3 +++ 1 file changed, 3 insertions(+) diff --git a/pkg/jobs/delegate_control_test.go b/pkg/jobs/delegate_control_test.go index 09ec82ea4a71..11b60d172666 100644 --- a/pkg/jobs/delegate_control_test.go +++ b/pkg/jobs/delegate_control_test.go @@ -23,6 +23,7 @@ import ( "github.com/cockroachdb/cockroach/pkg/security/username" "github.com/cockroachdb/cockroach/pkg/settings/cluster" "github.com/cockroachdb/cockroach/pkg/sql/sessiondata" + "github.com/cockroachdb/cockroach/pkg/testutils/skip" "github.com/cockroachdb/cockroach/pkg/util/leaktest" "github.com/cockroachdb/cockroach/pkg/util/log" "github.com/cockroachdb/cockroach/pkg/util/timeutil" @@ -338,6 +339,8 @@ func TestJobControlByType(t *testing.T) { defer log.Scope(t).Close(t) defer ResetConstructors()() + skip.UnderShort(t) // Test runs for more than 30s. + argsFn := func(args *base.TestServerArgs) { // Prevent registry from changing job state while running this test. interval := 24 * time.Hour From cd31dec3113a51b2397bf1505e72d37571fc41dc Mon Sep 17 00:00:00 2001 From: Raphael 'kena' Poss Date: Sat, 8 Apr 2023 12:54:00 +0200 Subject: [PATCH 4/5] jobs: clean up always when hopping to the next subtest in TestJobControlByType This simplifies reasoning in TestJobControlByType. Release note: None --- pkg/jobs/delegate_control_test.go | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/pkg/jobs/delegate_control_test.go b/pkg/jobs/delegate_control_test.go index 11b60d172666..3723c976057c 100644 --- a/pkg/jobs/delegate_control_test.go +++ b/pkg/jobs/delegate_control_test.go @@ -433,6 +433,11 @@ func TestJobControlByType(t *testing.T) { } jobIdsClause := fmt.Sprint(strings.Join(jobIDStrings, ", ")) + defer func() { + // Clear the system.jobs table for the next test run. + th.sqlDB.Exec(t, fmt.Sprintf("DELETE FROM system.jobs WHERE id IN (%s)", jobIdsClause)) + th.sqlDB.Exec(t, fmt.Sprintf("DELETE FROM system.job_info WHERE job_id IN (%s)", jobIdsClause)) + }() // Execute the command and verify it is executed on the expected number of rows. numEffected, err := th.cfg.DB.Executor().ExecEx( @@ -463,10 +468,6 @@ func TestJobControlByType(t *testing.T) { ), ).Scan(&numJobs) require.Equal(t, numJobs, numExpectedJobsWithEndState) - - // Clear the system.jobs table for the next test run. - th.sqlDB.Exec(t, fmt.Sprintf("DELETE FROM system.jobs WHERE id IN (%s)", jobIdsClause)) - th.sqlDB.Exec(t, fmt.Sprintf("DELETE FROM system.job_info WHERE job_id IN (%s)", jobIdsClause)) }) } } From f59ae77afc7fbd3b1d7eb3448a5b688c659a7958 Mon Sep 17 00:00:00 2001 From: Raphael 'kena' Poss Date: Sat, 8 Apr 2023 12:56:32 +0200 Subject: [PATCH 5/5] jobs: add some more debugging to TestJobControlByType Release note: None --- pkg/jobs/delegate_control_test.go | 46 ++++++++++++++++++++++++++++--- 1 file changed, 42 insertions(+), 4 deletions(-) diff --git a/pkg/jobs/delegate_control_test.go b/pkg/jobs/delegate_control_test.go index 3723c976057c..ef68b09bab98 100644 --- a/pkg/jobs/delegate_control_test.go +++ b/pkg/jobs/delegate_control_test.go @@ -15,6 +15,7 @@ import ( "fmt" "strings" "testing" + "text/tabwriter" "time" "github.com/cockroachdb/cockroach/pkg/base" @@ -349,6 +350,31 @@ func TestJobControlByType(t *testing.T) { th, cleanup := newTestHelperForTables(t, jobstest.UseSystemTables, argsFn) defer cleanup() + delayedShowJobs := func(t *testing.T, query string) fmt.Stringer { + return delayedStringer{ + func() string { + qrows := th.sqlDB.QueryStr(t, query) + jrows := th.sqlDB.QueryStr(t, "TABLE crdb_internal.jobs") + + var buf strings.Builder + tw := tabwriter.NewWriter(&buf, 2, 1, 2, ' ', 0) + fmt.Fprintf(tw, "output of %q:\n", query) + for _, row := range qrows { + fmt.Fprintln(tw, strings.Join(row, "\t")) + } + _ = tw.Flush() + + tw = tabwriter.NewWriter(&buf, 2, 1, 2, ' ', 0) + fmt.Fprintf(tw, "current jobs:\n") + for _, row := range jrows { + fmt.Fprintln(tw, strings.Join(row, "\t")) + } + _ = tw.Flush() + return buf.String() + }, + } + } + registry := th.server.JobRegistry().(*Registry) blockResume := make(chan struct{}) defer close(blockResume) @@ -400,6 +426,7 @@ func TestJobControlByType(t *testing.T) { {"cancel", []Status{StatusPending, StatusRunning, StatusPaused}, StatusCancelRequested}, } { commandQuery := fmt.Sprintf("%s ALL %s JOBS", tc.command, jobspbTypeToString[jobType]) + subJobs := "SHOW JOBS SELECT id FROM system.jobs WHERE job_type='" + jobspbTypeToString[jobType] + "'" t.Run(commandQuery, func(t *testing.T) { var jobIDStrings []string @@ -428,11 +455,12 @@ func TestJobControlByType(t *testing.T) { _, err := registry.CreateAdoptableJobWithTxn(context.Background(), record, jobID, nil /* txn */) require.NoError(t, err) th.sqlDB.Exec(t, "UPDATE system.jobs SET status=$1 WHERE id=$2", status, jobID) + t.Logf("created job %d (%T) with status %s", jobID, record.Details, status) } } } - jobIdsClause := fmt.Sprint(strings.Join(jobIDStrings, ", ")) + jobIdsClause := strings.Join(jobIDStrings, ", ") defer func() { // Clear the system.jobs table for the next test run. th.sqlDB.Exec(t, fmt.Sprintf("DELETE FROM system.jobs WHERE id IN (%s)", jobIdsClause)) @@ -447,11 +475,11 @@ func TestJobControlByType(t *testing.T) { sessiondata.RootUserSessionDataOverride, commandQuery, ) - require.NoError(t, err) + require.NoError(t, err, "%s", delayedShowJobs(t, subJobs)) // Jobs in the starting state should be affected. numExpectedJobsAffected := numJobsPerStatus * len(tc.startingStates) - require.Equal(t, numExpectedJobsAffected, numEffected) + require.Equal(t, numExpectedJobsAffected, numEffected, "%s", delayedShowJobs(t, subJobs)) // Both the affected jobs + the jobs originally in the target state should be in that state. numExpectedJobsWithEndState := numExpectedJobsAffected + numJobsPerStatus @@ -467,8 +495,18 @@ func TestJobControlByType(t *testing.T) { tc.endState, jobType, jobIdsClause, ), ).Scan(&numJobs) - require.Equal(t, numJobs, numExpectedJobsWithEndState) + require.Equal(t, numJobs, numExpectedJobsWithEndState, "%s", delayedShowJobs(t, subJobs)) }) } } } + +// delayedStringer is a stringer that delays the construction of its +// result string to the point String() is called. +type delayedStringer struct { + stringFn func() string +} + +func (d delayedStringer) String() string { + return d.stringFn() +}