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.

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

[close #287] fix alertmanager for tikv-cdc #288

Merged
merged 3 commits into from
Nov 3, 2022

Conversation

zeminzhou
Copy link
Contributor

Signed-off-by: zeminzhou [email protected]

What problem does this PR solve?

Issue Number: close #287

Problem Description: TBD

What is changed and how does it work?

ticdc -> tikv-cdc

Code changes

  • No code

Check List for Tests

This PR has been tested by at least one of the following methods:

  • No code

Side effects

  • No side effects

Related changes

  • No related changes

Copy link
Contributor

@haojinming haojinming left a comment

Choose a reason for hiding this comment

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

LGTM

@@ -2,111 +2,99 @@ groups:
- name: alert.rules
rules:
- alert: cdc_multiple_owners
expr: sum(rate(ticdc_owner_ownership_counter[30s])) >= 2
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggest:

  1. Rename the file to tikv-cdc.rules.yml.
  2. Change the alert names to avoid conflict with TiCDC.
  3. Change "cdc" to "TiKV-CDC" in summary to avoid misunstanding of user.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good suggest, modified, thanks!

@codecov
Copy link

codecov bot commented Nov 3, 2022

Codecov Report

Merging #288 (aa5a468) into main (c4c08d6) will increase coverage by 0.2421%.
The diff coverage is n/a.

❗ Current head aa5a468 differs from pull request most recent head 383c161. Consider uploading reports for the commit 383c161 to get more accurate results

Additional details and impacted files

Impacted file tree graph

@@               Coverage Diff                @@
##               main       #288        +/-   ##
================================================
+ Coverage   54.4448%   54.6869%   +0.2421%     
================================================
  Files           238        238                
  Lines         20237      20237                
================================================
+ Hits          11018      11067        +49     
+ Misses         8341       8273        -68     
- Partials        878        897        +19     
Flag Coverage Δ *Carryforward flag
br 39.5359% <ø> (ø) Carriedforward from c4c08d6
cdc 61.6611% <ø> (+0.3535%) ⬆️

*This pull request uses carry forward flags. Click here to find out more.

Impacted Files Coverage Δ
cdc/pkg/retry/retry_with_opt.go 90.9090% <0.0000%> (-4.5455%) ⬇️
cdc/cdc/sorter/unified/heap_sorter.go 85.7142% <0.0000%> (-1.5307%) ⬇️
cdc/cdc/processor/processor.go 65.3465% <0.0000%> (+0.7425%) ⬆️
cdc/pkg/orchestrator/etcd_worker.go 79.4117% <0.0000%> (+0.8403%) ⬆️
cdc/cdc/sorter/unified/merger.go 69.5792% <0.0000%> (+0.9708%) ⬆️
cdc/cdc/sorter/unified/file_backend.go 53.9473% <0.0000%> (+20.1754%) ⬆️

Signed-off-by: zeminzhou <[email protected]>
Signed-off-by: zeminzhou <[email protected]>
Copy link
Collaborator

@pingyu pingyu left a comment

Choose a reason for hiding this comment

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

LGTM~

@pingyu pingyu enabled auto-merge (squash) November 3, 2022 14:47
@pingyu
Copy link
Collaborator

pingyu commented Nov 3, 2022

/verify

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

cdc: cdc new alertmanager
3 participants