Skip to content
This repository has been archived by the owner on Jul 24, 2024. It is now read-only.

stats: disable stats by default #693

Merged
merged 4 commits into from
Jan 14, 2021
Merged

stats: disable stats by default #693

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
f063ed6
stats: disable stats by default
3pointer Jan 14, 2021
85c9950
add test for disable stats
3pointer Jan 14, 2021
58cb669
Merge branch 'master' into disable_stats
3pointer Jan 14, 2021
eb300d6
set statsLease to -1 in restore
3pointer Jan 14, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions cmd/backup.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import (
"github.com/pingcap/errors"
"github.com/pingcap/log"
"github.com/pingcap/tidb/ddl"
"github.com/pingcap/tidb/session"
"github.com/spf13/cobra"
"go.uber.org/zap"
"sourcegraph.com/sourcegraph/appdash"
Expand All @@ -30,6 +31,11 @@ func runBackupCommand(command *cobra.Command, cmdName string) error {
ctx, store = trace.TracerStartSpan(ctx)
defer trace.TracerFinishSpan(ctx, store)
}
if cfg.IgnoreStats {
// Do not run stat worker in BR.
session.DisableStats4Test()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about restore?

Copy link
Collaborator Author

@3pointer 3pointer Jan 14, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not easy to distinguish whether we should DisableStats4Test in restoration. I had two ideas

  1. Read backupmeta and check all tables whether stats != nil. but we should do it carefully, because read backupmeta is in task.runRestore and we cannot run DisableStats4Test otherwise it will influence BR via SQL.

  2. Change kvproto to record the status of --ignore-stats. and this will involve other repo.

So both of them will change a lot. we need to confirm the cost of DisableStats4Test and is it worth for the above changes?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LoadStatsFromJson only execute SQL to storage. so we can disable stats loop directly.

}

if err := task.RunBackup(ctx, tidbGlue, cmdName, &cfg); err != nil {
log.Error("failed to backup", zap.Error(err))
return errors.Trace(err)
Expand Down
6 changes: 5 additions & 1 deletion pkg/task/backup.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,7 +83,11 @@ func DefineBackupFlags(flags *pflag.FlagSet) {
// This flag can impact the online cluster, so hide it in case of abuse.
_ = flags.MarkHidden(flagRemoveSchedulers)

flags.Bool(flagIgnoreStats, false,
// Disable stats by default. because of
// 1. DumpStatsToJson is not stable
// 2. It increases memory usage may cause BR OOM
// TODO: we need a better way to backup/restore stats.
flags.Bool(flagIgnoreStats, true,
"ignore backup stats, used for test")
// This flag is used for test. we should backup stats all the time.
_ = flags.MarkHidden(flagIgnoreStats)
Expand Down
21 changes: 19 additions & 2 deletions tests/br_full_ddl/run.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,12 +66,12 @@ run_sql "analyze table $DB.$TABLE;"
curl $TIDB_IP:10080/stats/dump/$DB/$TABLE | jq '.columns.field0' | jq 'del(.last_update_version)' > $BACKUP_STAT

# backup full
echo "backup start..."
echo "backup start with stats..."
# Do not log to terminal
unset BR_LOG_TO_TERM
cluster_index_before_backup=$(run_sql "show variables like '%cluster%';" | awk '{print $2}')

run_br --pd $PD_ADDR backup full -s "local://$TEST_DIR/$DB" --ratelimit 5 --concurrency 4 --log-file $LOG || cat $LOG
run_br --pd $PD_ADDR backup full -s "local://$TEST_DIR/$DB" --ratelimit 5 --concurrency 4 --log-file $LOG --ignore-stats=false || cat $LOG
checksum_count=$(cat $LOG | grep "checksum success" | wc -l | xargs)

if [ "${checksum_count}" != "1" ];then
Expand All @@ -80,6 +80,9 @@ if [ "${checksum_count}" != "1" ];then
exit 1
fi

echo "backup start without stats..."
run_br --pd $PD_ADDR backup full -s "local://$TEST_DIR/${DB}_disable_stats" --concurrency 4

run_sql "DROP DATABASE $DB;"

cluster_index_before_restore=$(run_sql "show variables like '%cluster%';" | awk '{print $2}')
Expand All @@ -91,6 +94,20 @@ if [[ "${cluster_index_before_backup}" != "${cluster_index_before_restore}" ]];
exit 1
fi

echo "restore full without stats..."
run_br restore full -s "local://$TEST_DIR/${DB}_disable_stats" --pd $PD_ADDR
curl $TIDB_IP:10080/stats/dump/$DB/$TABLE | jq '.columns.field0' | jq 'del(.last_update_version)' > $RESOTRE_STAT

# stats should not be equal because we disable stats by default.
if diff -q $BACKUP_STAT $RESOTRE_STAT > /dev/null
then
echo "TEST: [$TEST_NAME] fail due to stats are equal"
exit 1
fi

# clear restore environment
run_sql "DROP DATABASE $DB;"

# restore full
echo "restore start..."
export GO_FAILPOINTS="github.com/pingcap/br/pkg/pdutil/PDEnabledPauseConfig=return(true)"
Expand Down