-
Notifications
You must be signed in to change notification settings - Fork 5.8k
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.
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
streamhelper/advancer: Fix owner transfer metric #42422
Conversation
[REVIEW NOTIFICATION] This pull request has not been approved. To complete the pull request process, please ask the reviewers in the list to review by filling The full list of commands accepted by this bot can be found here. Reviewer can indicate their review by submitting an approval review. |
Signed-off-by: hillium <yujuncen@pingcap.com>
b318fd2
to
474dc77
Compare
Signed-off-by: hillium <yujuncen@pingcap.com>
@@ -48,6 +48,7 @@ func (c *CheckpointAdvancer) Name() string { | |||
|
|||
func (c *CheckpointAdvancer) onStop() { | |||
metrics.AdvancerOwner.Set(0.0) | |||
metrics.LastCheckpoint.Reset() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about other metrics? should also reset them?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think perhaps we don't need to reset them before some problems with them evaluated. 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we put a comment here saying other metrics potentially need to be audit as well? And maybe a comment describing why do we need to reset checkpoint for this issue? I don't have context on this so just looking at the code I'm very clear what needs to be done. Thanks!
/test unit-test
…On 3/21/23, Ti Chi Robot ***@***.***> wrote:
@YuJuncen: The following test **failed**, say `/retest` to rerun all failed
tests or `/retest-required` to rerun all mandatory failed tests:
Test name | Commit | Details | Required | Rerun command
--- | --- | --- | --- | ---
idc-jenkins-ci-tidb/unit-test | 4ad66ba |
[link](https://do.pingcap.net/jenkins/job/pingcap/job/tidb/job/ghpr_unit_test/20094/display/redirect)
| true | `/test unit-test`
[Full PR test
history](https://prow.tidb.net/pr-history?org=pingcap&repo=tidb&pr=42422).
[Your PR
dashboard](https://prow.tidb.net/pr?query=is%3Apr%20state%3Aopen%20author%3AYuJuncen).
<details>
Instructions for interacting with me using PR comments are available
[here](https://git.k8s.io/community/contributors/guide/pull-requests.md).
If you have questions or suggestions related to my behavior, please file an
issue against the
[kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:)
repository. I understand the commands that are listed
[here](https://go.k8s.io/bot-commands).
</details>
<!-- test report -->
--
Reply to this email directly or view it on GitHub:
#42422 (comment)
You are receiving this because you were mentioned.
Message ID: ***@***.***>
|
/test unit-test |
@YuJuncen: Cannot trigger testing until a trusted user reviews the PR and leaves an In response to this:
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-sigs/prow repository. |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: 3pointer, Leavrth The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
@YuJuncen: The following tests failed, say
Full PR test history. Your PR dashboard. 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-sigs/prow repository. I understand the commands that are listed here. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #42422 +/- ##
================================================
+ Coverage 73.6423% 80.0476% +6.4052%
================================================
Files 1167 2858 +1691
Lines 365844 834447 +468603
================================================
+ Hits 269416 667955 +398539
- Misses 79076 140293 +61217
- Partials 17352 26199 +8847 |
/retest |
@BornChanger: Cannot trigger testing until a trusted user reviews the PR and leaves an In response to this:
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-sigs/prow repository. |
Thanks for the fix! |
/retest |
@YuJuncen: Cannot trigger testing until a trusted user reviews the PR and leaves an In response to this:
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-sigs/prow repository. |
What problem does this PR solve?
Issue Number: close #42419
Problem Summary:
When the owner get evicted but not restarted, its checkpoint metric may consist.
What is changed and how it works?
Check List
Tests
(I think this could be covered by the existing tests in AdvancerDaemon.)
(TBD)
Release note
Please refer to Release Notes Language Style Guide to write a quality release note.