From a62f9e988b3341729f1b2212ebf228ed5914f454 Mon Sep 17 00:00:00 2001 From: Andy Yang Date: Mon, 25 Mar 2024 14:40:07 -0400 Subject: [PATCH 1/2] changefeedccl: add logging for bulk oracle/rebalancing during planning This patch adds a log line to note when the new bulk oracle is used and makes an existing log line for rebalancing active even when verbose logging is not on to aid with future debugging. Release note: None --- pkg/ccl/changefeedccl/changefeed_dist.go | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/pkg/ccl/changefeedccl/changefeed_dist.go b/pkg/ccl/changefeedccl/changefeed_dist.go index a59e3b45921d..e054c23e98e5 100644 --- a/pkg/ccl/changefeedccl/changefeed_dist.go +++ b/pkg/ccl/changefeedccl/changefeed_dist.go @@ -396,6 +396,7 @@ func makePlan( evalCtx := execCtx.ExtendedEvalContext() oracle := replicaoracle.NewOracle(replicaOracleChoice, dsp.ReplicaOracleConfig(locFilter)) if useBulkOracle.Get(&evalCtx.Settings.SV) { + log.Infof(ctx, "using bulk oracle for DistSQL planning") oracle = kvfollowerreadsccl.NewBulkOracle(dsp.ReplicaOracleConfig(evalCtx.Locality), locFilter, kvfollowerreadsccl.StreakConfig{}) } planCtx := dsp.NewPlanningCtxWithOracle(ctx, execCtx.ExtendedEvalContext(), nil, /* planner */ @@ -410,9 +411,7 @@ func makePlan( switch { case distMode == sql.LocalDistribution || rangeDistribution == int64(defaultDistribution): case rangeDistribution == int64(balancedSimpleDistribution): - if log.ExpensiveLogEnabled(ctx, 2) { - log.Infof(ctx, "rebalancing ranges using balanced simple distribution") - } + log.Infof(ctx, "rebalancing ranges using balanced simple distribution") sender := execCtx.ExecCfg().DB.NonTransactionalSender() distSender := sender.(*kv.CrossRangeTxnWrapperSender).Wrapped().(*kvcoord.DistSender) ri := kvcoord.MakeRangeIterator(distSender) From 07e867f3dcfc74d553fd19737009af02d8fd294c Mon Sep 17 00:00:00 2001 From: Jackson Owens Date: Tue, 26 Mar 2024 16:29:26 -0400 Subject: [PATCH 2/2] server: clean up formatting of store props logged on start On start Cockroach logs some information about the stores it initialized. Previously, the store properties were included through logging the roachpb.StoreProperties struct with a +v format specifier. Since the WAL failover path is a pointer to a string, the address of the string was printed rather than its contents. This commit implements redact.SafeFormatter for the roachpb.StoreProperties type and uses it to fix this bug. Epic: none Release note: none --- pkg/roachpb/data.go | 23 +++++++++++++++++++++ pkg/roachpb/data_test.go | 44 ++++++++++++++++++++++++++++++++++++++++ pkg/server/config.go | 2 +- 3 files changed, 68 insertions(+), 1 deletion(-) diff --git a/pkg/roachpb/data.go b/pkg/roachpb/data.go index 000d8ba3d603..3d2880a6ce2d 100644 --- a/pkg/roachpb/data.go +++ b/pkg/roachpb/data.go @@ -902,6 +902,29 @@ func (v Value) PrettyPrint() (ret string) { return buf.String() } +// SafeFormat implements the redact.SafeFormatter interface. +func (sp StoreProperties) SafeFormat(w redact.SafePrinter, _ rune) { + w.SafeString(redact.SafeString(sp.Dir)) + w.SafeString(":") + if sp.ReadOnly { + w.SafeString(" ro") + } else { + w.SafeString(" rw") + } + w.Printf(" encrypted=%t", sp.Encrypted) + if sp.WalFailoverPath != nil { + w.Printf(" wal_failover_path=%s", redact.SafeString(*sp.WalFailoverPath)) + } + if sp.FileStoreProperties != nil { + w.SafeString(" fs:{") + w.Printf("bdev=%s", redact.SafeString(sp.FileStoreProperties.BlockDevice)) + w.Printf(" fstype=%s", redact.SafeString(sp.FileStoreProperties.FsType)) + w.Printf(" mountpoint=%s", redact.SafeString(sp.FileStoreProperties.MountPoint)) + w.Printf(" mountopts=%s", redact.SafeString(sp.FileStoreProperties.MountOptions)) + w.SafeString("}") + } +} + // Kind returns the kind of commit trigger as a string. func (ct InternalCommitTrigger) Kind() redact.SafeString { switch { diff --git a/pkg/roachpb/data_test.go b/pkg/roachpb/data_test.go index 6eab1f140843..17da1a31cf3a 100644 --- a/pkg/roachpb/data_test.go +++ b/pkg/roachpb/data_test.go @@ -367,6 +367,50 @@ func TestValueGetErrorsRedacted(t *testing.T) { require.Equal(t, string(redact.Sprintf("%s %s", err, "sensitive").Redact()), "value type is not INT: BYTES ‹×›") } +func TestStorePropertiesSafeFormat(t *testing.T) { + type testCase struct { + props *StoreProperties + formatted redact.RedactableString + } + + walFailoverPath := "/mnt/data2/cockroach/auxiliary/among-stores" + + testCases := []testCase{ + { + props: &StoreProperties{ + Dir: "/mnt/data1/cockroach", + Encrypted: true, + WalFailoverPath: &walFailoverPath, + FileStoreProperties: &FileStoreProperties{ + BlockDevice: "nvme1n1", + FsType: "ext4", + MountPoint: "/mnt/data1", + MountOptions: "rw,relatime", + }, + }, + formatted: "/mnt/data1/cockroach: rw encrypted=true wal_failover_path=/mnt/data2/cockroach/auxiliary/among-stores fs:{bdev=nvme1n1 fstype=ext4 mountpoint=/mnt/data1 mountopts=rw,relatime}", + }, + { + props: &StoreProperties{ + Dir: "/mnt/data3/cockroach", + ReadOnly: true, + Encrypted: false, + FileStoreProperties: &FileStoreProperties{ + BlockDevice: "nvme1n1", + FsType: "zfs", + MountPoint: "/mnt/data3", + MountOptions: "ro,relatime", + }, + }, + formatted: "/mnt/data3/cockroach: ro encrypted=false fs:{bdev=nvme1n1 fstype=zfs mountpoint=/mnt/data3 mountopts=ro,relatime}", + }, + } + for _, tc := range testCases { + got := redact.Sprintf("%s", tc.props) + require.Equal(t, tc.formatted, got) + } +} + func TestSetGetChecked(t *testing.T) { v := Value{} diff --git a/pkg/server/config.go b/pkg/server/config.go index e28390da945f..19d27ee95b36 100644 --- a/pkg/server/config.go +++ b/pkg/server/config.go @@ -861,7 +861,7 @@ func (cfg *Config) CreateEngines(ctx context.Context) (Engines, error) { // TODO(jackson): Refactor to either reference count references to the env, // or leave ownership with the caller of Open. storeEnvs[i] = nil - detail(redact.Sprintf("store %d: %+v", i, eng.Properties())) + detail(redact.Sprintf("store %d: %s", i, eng.Properties())) engines = append(engines, eng) }