Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

stats: disable stats by default #693

Merged
merged 4 commits into from
Jan 14, 2021
Merged

stats: disable stats by default #693

merged 4 commits into from
Jan 14, 2021

Conversation

3pointer
Copy link
Collaborator

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

What problem does this PR solve?

Disable backup stats by default. because of

  1. DumpStatsToJson is not stable
  2. It increases memory usage may cause BR OOM

What is changed and how it works?

  1. set cfg.IgnoreStats default to true
  2. if cfg.IgnoreStats is true, set statsLease to -1

Check List

Tests

  • Integration test

Related changes

  • Need to cherry-pick to the release branch
  • Need to update the documentation

Release Note

  • Disable backup stats by default to avoid BR reached OOM issue during backup.

@3pointer 3pointer added needs-cherry-pick-release-4.0 needs-cherry-pick-5.0-rc Priority/P0 Top priority issue. Must have an associated milestone labels Jan 14, 2021
@3pointer 3pointer changed the title WIP: stats: disable stats by default stats: disable stats by default Jan 14, 2021
overvenus
overvenus reviewed Jan 14, 2021
View reviewed changes
cmd/backup.go
@@ -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.

kennytm
kennytm reviewed Jan 14, 2021
View reviewed changes
Copy link
Collaborator

@kennytm kennytm left a comment

Choose a reason for hiding this comment

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

lgtm

@ti-srebot ti-srebot added the status/LGT1 LGTM1 label Jan 14, 2021
@3pointer
Copy link
Collaborator Author

Test restore with sysbench 3000 tables.
v4.0.8 mem inuse 61M
v4.0.9 mem inuse 88.96M
v4.0.9 + #693 mem inuse 81.66M

@overvenus
Copy link
Member

LGTM

@ti-srebot ti-srebot removed the status/LGT1 LGTM1 label Jan 14, 2021
@ti-srebot ti-srebot added the status/LGT2 LGTM2 label Jan 14, 2021
@3pointer 3pointer merged commit 87eb3ed into pingcap:master Jan 14, 2021
ti-srebot pushed a commit to ti-srebot/br that referenced this pull request Jan 14, 2021
@3pointer @ti-srebot
cherry pick pingcap#693 to release-4.0
40c59e8 
Signed-off-by: ti-srebot <ti-srebot@pingcap.com>
@ti-srebot ti-srebot mentioned this pull request Jan 14, 2021
Merged
@ti-srebot
Copy link
Contributor

cherry pick to release-4.0 in PR #696

ti-srebot pushed a commit to ti-srebot/br that referenced this pull request Jan 14, 2021
@3pointer @ti-srebot
cherry pick pingcap#693 to release-5.0-rc
b7fe252 
Signed-off-by: ti-srebot <ti-srebot@pingcap.com>
@ti-srebot ti-srebot mentioned this pull request Jan 14, 2021
Closed
@ti-srebot
Copy link
Contributor

cherry pick to release-5.0-rc in PR #697

@3pointer
Copy link
Collaborator Author

Test backup with sysbench 3000 tables.
v4.0.9 + #693 inuse Showing nodes accounting for 21.65MB, 72.98% of 29.66MB total
v4.0.9 inuse Showing nodes accounting for 30.05MB, 62.90% of 47.78MB total

We can reduce the memory inuse with this PR.

3pointer added a commit that referenced this pull request Jan 14, 2021
@ti-srebot @3pointer
stats: disable stats by default (#693) (#696)
e8a906b 
* cherry pick #693 to release-4.0

Signed-off-by: ti-srebot <ti-srebot@pingcap.com>

Co-authored-by: 3pointer <luancheng@pingcap.com>
@kennytm kennytm mentioned this pull request Jan 18, 2021
Closed
@tennix tennix mentioned this pull request Jan 18, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kennytm kennytm kennytm left review comments

@overvenus overvenus overvenus left review comments

@ti-srebot ti-srebot ti-srebot approved these changes

Assignees
No one assigned
Labels
needs-cherry-pick-release-4.0 Priority/P0 Top priority issue. Must have an associated milestone release-blocker status/LGT2 LGTM2
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@3pointer @overvenus @ti-srebot @kennytm