*: add defer to logs the panic reason and stack #6123

HuSharp · 2023-03-09T03:06:32Z

What problem does this PR solve?

Issue Number: Ref #6099

What is changed and how does it work?

add defer logs the panic reason and stack

Check List

Tests

Unit test
Integration test
Manual test (add detailed scripts or steps below)
No code

Release note

None.

ti-chi-bot · 2023-03-09T03:06:34Z

[REVIEW NOTIFICATION]

This pull request has been approved by:

bufferflies
rleungx

To complete the pull request process, please ask the reviewers in the list to review by filling /cc @reviewer in the comment.
After your PR has acquired the required number of LGTMs, you can assign this pull request to the committer in the list by filling /assign @committer in the comment to help you merge this pull request.

The full list of commands accepted by this bot can be found here.

Reviewer can indicate their review by submitting an approval review.
Reviewer can cancel approval by submitting a request changes review.

ti-chi-bot · 2023-03-09T03:06:41Z

Hi @HuSharp. Thanks for your PR.

I'm waiting for a tikv member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

codecov · 2023-03-09T03:13:59Z

Codecov Report

Patch coverage: 85.71% and project coverage change: +0.02 🎉

Comparison is base (35e2e11) 74.35% compared to head (9504eec) 74.37%.

❗ Current head 9504eec differs from pull request most recent head 22c0e31. Consider uploading reports for the commit 22c0e31 to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #6123      +/-   ##
==========================================
+ Coverage   74.35%   74.37%   +0.02%     
==========================================
  Files         389      389              
  Lines       38128    38134       +6     
==========================================
+ Hits        28349    28364      +15     
+ Misses       7254     7243      -11     
- Partials     2525     2527       +2

Flag	Coverage Δ
unittests	`74.37% <85.71%> (+0.02%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
pkg/gctuner/memory_limit_tuner.go	`45.65% <0.00%> (-1.02%)`	⬇️
pkg/mcs/tso/server/grpc_service.go	`12.00% <0.00%> (-0.14%)`	⬇️
pkg/mcs/tso/server/server.go	`47.05% <80.00%> (+0.54%)`	⬆️
pkg/cache/ttl.go	`97.95% <100.00%> (+0.02%)`	⬆️
pkg/election/lease.go	`84.50% <100.00%> (+0.44%)`	⬆️
pkg/mcs/discovery/register.go	`48.97% <100.00%> (+1.06%)`	⬆️
pkg/mcs/resource_manager/server/manager.go	`82.20% <100.00%> (+0.44%)`	⬆️
pkg/mcs/resource_manager/server/server.go	`54.62% <100.00%> (+0.81%)`	⬆️
pkg/schedule/region_scatterer.go	`79.10% <100.00%> (+1.69%)`	⬆️
pkg/systimemon/systimemon.go	`100.00% <100.00%> (ø)`
... and 3 more

... and 28 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

bufferflies · 2023-03-09T03:16:06Z

/need_ok_test

HuSharp · 2023-03-09T03:31:52Z

/need_ok_test

maybe /ok-to-test

rleungx · 2023-03-09T03:40:05Z

/ok-to-test

rleungx · 2023-03-09T09:30:41Z

pkg/mcs/resource_manager/server/manager.go

@@ -121,7 +122,10 @@ func (m *Manager) Init(ctx context.Context) {
 	m.storage.LoadResourceGroupStates(tokenHandler)
 	// Start the background metrics flusher.
 	go m.backgroundMetricsFlush(ctx)
-	go m.persistLoop(ctx)
+	go func() {


What is the difference between backgroundMetricsFlush and persistLoop?

persistLoop function requires a look at the code to know that it is a goroutine and is not used anywhere else.
To avoid adding logpanic to confuse people, it was changed to this

server/server.go

Signed-off-by: husharp <jinhao.hu@pingcap.com>

HuSharp · 2023-03-13T06:02:17Z

@bufferflies @rleungx can u leave a merge label? thx!

rleungx · 2023-03-13T06:54:39Z

/merge

ti-chi-bot · 2023-03-13T06:54:41Z

@rleungx: It seems you want to merge this PR, I will help you trigger all the tests:

/run-all-tests

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the ti-community-infra/tichi repository.

ti-chi-bot · 2023-03-13T06:54:43Z

This pull request has been accepted and is ready to merge.

Commit hash: 9504eec

ti-chi-bot · 2023-03-13T06:54:56Z

@HuSharp: Your PR was out of date, I have automatically updated it for you.

If the CI test fails, you just re-trigger the test that failed and the bot will merge the PR for you after the CI passes.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the ti-community-infra/tichi repository.

ref tikv#6099 Signed-off-by: ti-chi-bot <ti-community-prow-bot@tidb.io>

ti-chi-bot · 2023-03-13T07:07:38Z

In response to a cherrypick label: new pull request created to branch release-6.1: #6153.

ref tikv#6099 Signed-off-by: ti-chi-bot <ti-community-prow-bot@tidb.io>

ti-chi-bot · 2023-03-13T07:08:17Z

In response to a cherrypick label: new pull request created to branch release-6.5: #6154.

ref #6099, ref #6123 add defer logs the panic reason and stack Signed-off-by: husharp <jinhao.hu@pingcap.com> Co-authored-by: husharp <jinhao.hu@pingcap.com>

ref #6099, ref #6123 add defer logs the panic reason and stack Signed-off-by: husharp <jinhao.hu@pingcap.com> Co-authored-by: husharp <jinhao.hu@pingcap.com> Co-authored-by: Hu# <jinhao.hu@pingcap.com> Co-authored-by: ti-chi-bot[bot] <108142056+ti-chi-bot[bot]@users.noreply.github.com>

* fix tso inconsistent problem Signed-off-by: Ryan Leung <rleungx@gmail.com> * *: fix the missing log panic (tikv#6325) close tikv#6257 Signed-off-by: Ryan Leung <rleungx@gmail.com> * *: add defer to logs the panic reason and stack (tikv#6123) ref tikv#6099 add defer logs the panic reason and stack Signed-off-by: husharp <jinhao.hu@pingcap.com> Co-authored-by: Ti Chi Robot <ti-community-prow-bot@tidb.io> --------- Signed-off-by: Ryan Leung <rleungx@gmail.com> Co-authored-by: Hu# <jinhao.hu@pingcap.com> Co-authored-by: Ti Chi Robot <ti-community-prow-bot@tidb.io>

ti-chi-bot added do-not-merge/needs-linked-issue do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. release-note-none needs-ok-to-test labels Mar 9, 2023

ti-chi-bot requested review from disksing and rleungx March 9, 2023 03:06

ti-chi-bot added do-not-merge/needs-triage-completed and removed do-not-merge/needs-linked-issue labels Mar 9, 2023

HuSharp force-pushed the add_log_defer branch from 631f798 to 9d50f0f Compare March 9, 2023 03:29

ti-chi-bot added ok-to-test needs-cherry-pick-release-6.5 and removed needs-ok-to-test do-not-merge/needs-triage-completed labels Mar 9, 2023

HuSharp marked this pull request as ready for review March 9, 2023 09:24

ti-chi-bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Mar 9, 2023

HuSharp changed the title *: add defer to logs the panic reason and stack *: repair PD maybe panic after scatter and add defer to logs the panic reason and stack Mar 9, 2023

rleungx reviewed Mar 9, 2023

View reviewed changes

server/server.go Outdated Show resolved Hide resolved

ti-chi-bot added the needs-cherry-pick-release-6.1 label Mar 9, 2023

HuSharp changed the title *: repair PD maybe panic after scatter and add defer to logs the panic reason and stack *: add defer to logs the panic reason and stack Mar 9, 2023

ti-chi-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 11, 2023

HuSharp added 3 commits March 13, 2023 09:43

add log defer

6088878

Signed-off-by: husharp <jinhao.hu@pingcap.com>

merge master

a157171

Signed-off-by: husharp <jinhao.hu@pingcap.com>

remove monitor log panic

702ca48

Signed-off-by: husharp <jinhao.hu@pingcap.com>

HuSharp force-pushed the add_log_defer branch from 1e8c9cc to 702ca48 Compare March 13, 2023 01:53

ti-chi-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 13, 2023

bufferflies approved these changes Mar 13, 2023

View reviewed changes

ti-chi-bot added the status/LGT1 Indicates that a PR has LGTM 1. label Mar 13, 2023

rleungx approved these changes Mar 13, 2023

View reviewed changes

ti-chi-bot added status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels Mar 13, 2023

Merge branch 'master' into add_log_defer

9504eec

ti-chi-bot added the status/can-merge Indicates a PR has been approved by a committer. label Mar 13, 2023

Merge branch 'master' into add_log_defer

22c0e31

ti-chi-bot merged commit 5bdfa71 into tikv:master Mar 13, 2023
16 checks passed

ti-chi-bot mentioned this pull request Mar 13, 2023

*: add defer to logs the panic reason and stack (#6123) #6153

Merged

ti-chi-bot pushed a commit to ti-chi-bot/pd that referenced this pull request Mar 13, 2023

This is an automated cherry-pick of tikv#6123

b0567e9

ref tikv#6099 Signed-off-by: ti-chi-bot <ti-community-prow-bot@tidb.io>

ti-chi-bot pushed a commit to ti-chi-bot/pd that referenced this pull request Mar 13, 2023

This is an automated cherry-pick of tikv#6123

6276e36

ref tikv#6099 Signed-off-by: ti-chi-bot <ti-community-prow-bot@tidb.io>

ti-chi-bot mentioned this pull request Mar 13, 2023

*: add defer to logs the panic reason and stack (#6123) #6154

Merged

HuSharp deleted the add_log_defer branch March 13, 2023 07:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

*: add defer to logs the panic reason and stack #6123

*: add defer to logs the panic reason and stack #6123

HuSharp commented Mar 9, 2023 •

edited

ti-chi-bot commented Mar 9, 2023 •

edited

ti-chi-bot commented Mar 9, 2023

codecov bot commented Mar 9, 2023 •

edited

bufferflies commented Mar 9, 2023

HuSharp commented Mar 9, 2023 •

edited

rleungx commented Mar 9, 2023

rleungx Mar 9, 2023

HuSharp Mar 9, 2023

HuSharp commented Mar 13, 2023

rleungx commented Mar 13, 2023

ti-chi-bot commented Mar 13, 2023

ti-chi-bot commented Mar 13, 2023

ti-chi-bot commented Mar 13, 2023

ti-chi-bot commented Mar 13, 2023

ti-chi-bot commented Mar 13, 2023

*: add defer to logs the panic reason and stack #6123

*: add defer to logs the panic reason and stack #6123

Conversation

HuSharp commented Mar 9, 2023 • edited

What problem does this PR solve?

What is changed and how does it work?

Check List

Release note

ti-chi-bot commented Mar 9, 2023 • edited

ti-chi-bot commented Mar 9, 2023

codecov bot commented Mar 9, 2023 • edited

Codecov Report

bufferflies commented Mar 9, 2023

HuSharp commented Mar 9, 2023 • edited

rleungx commented Mar 9, 2023

rleungx Mar 9, 2023

Choose a reason for hiding this comment

HuSharp Mar 9, 2023

Choose a reason for hiding this comment

HuSharp commented Mar 13, 2023

rleungx commented Mar 13, 2023

ti-chi-bot commented Mar 13, 2023

ti-chi-bot commented Mar 13, 2023

ti-chi-bot commented Mar 13, 2023

ti-chi-bot commented Mar 13, 2023

ti-chi-bot commented Mar 13, 2023

HuSharp commented Mar 9, 2023 •

edited

ti-chi-bot commented Mar 9, 2023 •

edited

codecov bot commented Mar 9, 2023 •

edited

HuSharp commented Mar 9, 2023 •

edited