spanconfig: squash benign data race

Fixes cockroachdb#75849. Release note: None
fqazi · Apr 4, 2022 · cc5d44a · cc5d44a
1 parent 1eb1842
commit cc5d44a
Show file tree

Hide file tree

Showing 3 changed files with 21 additions and 8 deletions.
diff --git a/pkg/migration/migrations/migrate_span_configs_test.go b/pkg/migration/migrations/migrate_span_configs_test.go
@@ -21,7 +21,6 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/server"
 	"github.com/cockroachdb/cockroach/pkg/spanconfig"
 	"github.com/cockroachdb/cockroach/pkg/testutils"
-	"github.com/cockroachdb/cockroach/pkg/testutils/skip"
 	"github.com/cockroachdb/cockroach/pkg/testutils/sqlutils"
 	"github.com/cockroachdb/cockroach/pkg/testutils/testcluster"
 	"github.com/cockroachdb/cockroach/pkg/util/leaktest"
@@ -33,7 +32,6 @@ import (
 // span config reconciliation attempt, blocking until it occurs.
 func TestEnsureSpanConfigReconciliation(t *testing.T) {
 	defer leaktest.AfterTest(t)()
-	skip.WithIssue(t, 75849, "flaky test")
 	defer log.Scope(t).Close(t)
 
 	ctx := context.Background()
@@ -99,6 +97,8 @@ func TestEnsureSpanConfigReconciliation(t *testing.T) {
 	}
 }
 
+// TestEnsureSpanConfigReconciliationMultiNode verifies that the span config
+// reconciliation migration works in a multi-node setting.
 func TestEnsureSpanConfigReconciliationMultiNode(t *testing.T) {
 	defer leaktest.AfterTest(t)()
 	defer log.Scope(t).Close(t)

diff --git a/pkg/spanconfig/spanconfigjob/BUILD.bazel b/pkg/spanconfig/spanconfigjob/BUILD.bazel
@@ -15,6 +15,7 @@ go_library(
         "//pkg/util/hlc",
         "//pkg/util/log",
         "//pkg/util/retry",
+        "//pkg/util/syncutil",
         "//pkg/util/timeutil",
         "@com_github_cockroachdb_errors//:errors",
     ],

diff --git a/pkg/spanconfig/spanconfigjob/job.go b/pkg/spanconfig/spanconfigjob/job.go
@@ -23,6 +23,7 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/util/hlc"
 	"github.com/cockroachdb/cockroach/pkg/util/log"
 	"github.com/cockroachdb/cockroach/pkg/util/retry"
+	"github.com/cockroachdb/cockroach/pkg/util/syncutil"
 	"github.com/cockroachdb/cockroach/pkg/util/timeutil"
 	"github.com/cockroachdb/errors"
 )
@@ -91,18 +92,25 @@ func (r *resumer) Resume(ctx context.Context, execCtxI interface{}) (jobErr erro
 	// timestamp.
 
 	settingValues := &execCtx.ExecCfg().Settings.SV
-	persistCheckpoints := util.Every(reconciliationJobCheckpointInterval.Get(settingValues))
+	persistCheckpointsMu := struct {
+		syncutil.Mutex
+		util.EveryN
+	}{}
+	persistCheckpointsMu.EveryN = util.Every(reconciliationJobCheckpointInterval.Get(settingValues))
+
 	reconciliationJobCheckpointInterval.SetOnChange(settingValues, func(ctx context.Context) {
-		persistCheckpoints = util.Every(reconciliationJobCheckpointInterval.Get(settingValues))
+		persistCheckpointsMu.Lock()
+		defer persistCheckpointsMu.Unlock()
+		persistCheckpointsMu.EveryN = util.Every(reconciliationJobCheckpointInterval.Get(settingValues))
 	})
 
-	shouldPersistCheckpoint := true
+	checkpointingDisabled := false
 	shouldSkipRetry := false
 	var onCheckpointInterceptor func() error
 
 	if knobs := execCtx.ExecCfg().SpanConfigTestingKnobs; knobs != nil {
 		if knobs.JobDisablePersistingCheckpoints {
-			shouldPersistCheckpoint = false
+			checkpointingDisabled = true
 		}
 		shouldSkipRetry = knobs.JobDisableInternalRetry
 		onCheckpointInterceptor = knobs.JobOnCheckpointInterceptor
@@ -125,10 +133,14 @@ func (r *resumer) Resume(ctx context.Context, execCtxI interface{}) (jobErr erro
 				}
 			}
 
-			if !shouldPersistCheckpoint {
+			if checkpointingDisabled {
 				return nil
 			}
-			if !persistCheckpoints.ShouldProcess(timeutil.Now()) {
+
+			persistCheckpointsMu.Lock()
+			shouldPersistCheckpoint := persistCheckpointsMu.ShouldProcess(timeutil.Now())
+			persistCheckpointsMu.Unlock()
+			if !shouldPersistCheckpoint {
 				return nil
 			}