sql: proper version gate sql stats #69592
Conversation
|
This change is |
Azhng
force-pushed
the
sqlstats-version-gating
branch
from
f44ef78
to
e20f0af
Compare
There was a problem hiding this comment.
Reviewable status:
complete! 0 of 0 LGTMs obtained (waiting on @Azhng)
pkg/sql/sqlstats/persistedsqlstats/scheduled_job_monitor.go, line 145 at r1 (raw file):
func (j *jobMonitor) ensureSchedule(ctx context.Context) { if !j.isVersionCompatible(ctx) { log.Error(ctx, "cannot create sql stats scheduled compaction job because current cluster version is too low")
This isn't an error, right? I think this is a normal situation in the mixed version state. Maybe log.Infof at worst. Also, use Infof even with no parameters. Otherwise we'll redact it.
Azhng
force-pushed
the
sqlstats-version-gating
branch
from
e20f0af
to
412f804
Compare
There was a problem hiding this comment.
Reviewable status:
pkg/sql/sqlstats/persistedsqlstats/scheduled_job_monitor.go, line 145 at r1 (raw file):
Previously, ajwerner wrote…
This isn't an error, right? I think this is a normal situation in the mixed version state. Maybe
log.Infofat worst. Also, useInfofeven with no parameters. Otherwise we'll redact it.
Ah I misunderstood the earlier comment on the issue. Switched to log.Infof
There was a problem hiding this comment.
Reviewable status:
pkg/sql/sqlstats/persistedsqlstats/scheduled_job_monitor.go, line 176 at r2 (raw file):
Quoted 5 lines of code…
clusterVersion := j.st.Version.ActiveVersionOrEmpty(ctx) if !clusterVersion.IsActive(clusterversion.SQLStatsCompactionScheduledJob) { return false } return true
Why not just st.Version.IsActive(ctx, clusterversion.SQLStatsCompactionScheduledJob)?
Azhng
force-pushed
the
sqlstats-version-gating
branch
from
412f804
to
4c5c0aa
Compare
There was a problem hiding this comment.
Reviewable status:
pkg/sql/sqlstats/persistedsqlstats/scheduled_job_monitor.go, line 176 at r2 (raw file):
Previously, ajwerner wrote…
clusterVersion := j.st.Version.ActiveVersionOrEmpty(ctx) if !clusterVersion.IsActive(clusterversion.SQLStatsCompactionScheduledJob) { return false } return trueWhy not just
st.Version.IsActive(ctx, clusterversion.SQLStatsCompactionScheduledJob)?
Done.
There was a problem hiding this comment.
Reviewable status:
pkg/sql/sqlstats/persistedsqlstats/scheduled_job_monitor.go, line 93 at r3 (raw file):
} if err := j.db.Txn(ctx, func(ctx context.Context, txn *kv.Txn) error { j.ensureSchedule(ctx)
It feels bizarre to me to do this inside of the transaction. How about doing this two lines up?
Azhng
force-pushed
the
sqlstats-version-gating
branch
from
4c5c0aa
to
8c3bbd6
Compare
There was a problem hiding this comment.
Reviewed 1 of 3 files at r1, 1 of 1 files at r4, all commit messages.
Reviewable status:
pkg/sql/sqlstats/persistedsqlstats/job_monitor_test.go, line 55 at r4 (raw file):
sqlDB.Exec(t, "SET CLUSTER SETTING sql.stats.cleanup.recurrence = '@daily'") // Wait for the change is picked up by the job monitor.
nit: change to be picked
Previously, SQL Stats's implementation for version gating is faulty. This means that SQL Stats's job monitor would attempt to start sql stats compaction job in an incompatible cluster. This commit fixed the faulty implementation. Resolves cockroachdb#69459 Resolves cockroachdb#69541 Resolves cockroachdb#69544 Resolves cockroachdb#69565 Release justification: Category 2: Bug fixes and low-risk updates to new functionality Release note: None
Azhng
force-pushed
the
sqlstats-version-gating
branch
from
8c3bbd6
to
3ea128d
Compare
There was a problem hiding this comment.
Reviewable status:
pkg/sql/sqlstats/persistedsqlstats/job_monitor_test.go, line 55 at r4 (raw file):
Previously, maryliag (Marylia Gutierrez) wrote…
nit: change to be picked
Done.
There was a problem hiding this comment.
Reviewable status:
|
TFTR! bors r=maryliag,ajwerner |
|
Build succeeded: |
Previously, SQL Stats's implementation for version gating is faulty.
This means that SQL Stats's job monitor would attempt to start sql
stats compaction job in an incompatible cluster.
This commit fixed the faulty implementation.
Resolves #69459
Resolves #69544
Resolves #69565
Release justification: Category 2: Bug fixes and low-risk updates to
new functionality
Release note: None