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

[fix #246] fix unified sorter #247

Merged
merged 5 commits into from
Oct 9, 2022
Merged

[fix #246] fix unified sorter #247

merged 5 commits into from
Oct 9, 2022

Conversation

zeminzhou
Copy link
Contributor

Signed-off-by: zeminzhou zhouzemin@pingcap.com

What problem does this PR solve?

  1. fix #issue246
  2. remove unused log
  3. fix metric for puller output resolvedts

Issue Number: close #246

Problem Description: TBD

What is changed and how does it work?

remove wrong print

Code changes

  • Has exported function/method change

Check List for Tests

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

  • Unit test
  • Integration test
  • Manual test

Signed-off-by: zeminzhou <zhouzemin@pingcap.com>
Signed-off-by: zeminzhou <zhouzemin@pingcap.com>
@zeminzhou zeminzhou requested review from pingyu and haojinming October 9, 2022 06:43
@zeminzhou
Copy link
Contributor Author

PTAL~

@@ -231,6 +227,7 @@ func (p *pullerImpl) Run(ctx context.Context) error {
if err != nil {
return errors.Trace(err)
}
metricTxnCollectCounterResolved.Inc()
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 to metricPullOutputCounterResolved. So as to the underlying prom metric name.
  2. Utilize kvEventCounter as the puller input counter. The input & output metrics will help us determine whether there is issue in puller module. For both kv & resolved-ts event.
  3. Add the input counter to grafana panel.

Signed-off-by: zeminzhou <zhouzemin@pingcap.com>
@codecov
Copy link

codecov bot commented Oct 9, 2022

Codecov Report

Merging #247 (eeeb535) into main (92d4381) will decrease coverage by 0.2118%.
The diff coverage is 87.5000%.

Additional details and impacted files

Impacted file tree graph

@@               Coverage Diff                @@
##               main       #247        +/-   ##
================================================
- Coverage   61.2425%   61.0307%   -0.2119%     
================================================
  Files           238        239         +1     
  Lines         20200      20180        -20     
================================================
- Hits          12371      12316        -55     
- Misses         6690       6743        +53     
+ Partials       1139       1121        -18     
Flag Coverage Δ *Carryforward flag
br 60.2203% <ø> (ø) Carriedforward from 9a98d42
cdc 61.4032% <87.5000%> (-0.3086%) ⬇️

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

Impacted Files Coverage Δ
cdc/cdc/model/kv_gen.go 45.0331% <ø> (-0.1815%) ⬇️
cdc/cdc/processor/pipeline/puller.go 0.0000% <ø> (ø)
cdc/cdc/processor/pipeline/sink.go 63.0434% <ø> (-2.4971%) ⬇️
cdc/cdc/processor/processor.go 64.6039% <ø> (-0.1744%) ⬇️
cdc/cdc/puller/metrics.go 0.0000% <0.0000%> (ø)
cdc/cdc/sink/buffer_sink.go 81.3084% <ø> (+1.3084%) ⬆️
cdc/cdc/sink/keyspan_sink.go 0.0000% <ø> (ø)
cdc/cdc/sink/tikv.go 80.6584% <ø> (-0.4661%) ⬇️
cdc/cdc/puller/puller.go 81.2500% <100.0000%> (+0.6048%) ⬆️
cdc/cdc/sorter/unified/file_backend.go 33.7719% <0.0000%> (-20.1755%) ⬇️
... and 9 more

@zeminzhou zeminzhou force-pushed the fix-sort branch 2 times, most recently from 7536910 to 87d8293 Compare October 9, 2022 09:18
Signed-off-by: zeminzhou <zhouzemin@pingcap.com>
@@ -6496,100 +6496,6 @@
"alignLevel": null
}
},
{
Copy link
Contributor

Choose a reason for hiding this comment

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

Removing a panel will leave a blank space, may need move all other panels or modify the metric json in next pr?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yesh, it will leave a blank space. I add it back, and remove it in next pr. Thanks

Signed-off-by: zeminzhou <zhouzemin@pingcap.com>
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.

Please paste the screenshot of modified TiKV-CDC panel of Grafana in PR content.
Rest LGTM~

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

@zeminzhou
Copy link
Contributor Author

image
image
image

@zeminzhou
Copy link
Contributor Author

Please paste the screenshot of modified TiKV-CDC panel of Grafana in PR content. Rest LGTM~

added, thanks

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 merged commit af3ebdc into tikv:main Oct 9, 2022
pingyu pushed a commit to pingyu/tikv-migration that referenced this pull request Oct 9, 2022
* fix unified sorter

Signed-off-by: zeminzhou <zhouzemin@pingcap.com>

* remove debug log

Signed-off-by: zeminzhou <zhouzemin@pingcap.com>

* add metric for the input event of puller

Signed-off-by: zeminzhou <zhouzemin@pingcap.com>

* remove puller input metric

Signed-off-by: zeminzhou <zhouzemin@pingcap.com>

* add back

Signed-off-by: zeminzhou <zhouzemin@pingcap.com>

Signed-off-by: zeminzhou <zhouzemin@pingcap.com>
pingyu pushed a commit to pingyu/tikv-migration that referenced this pull request Oct 9, 2022
* fix unified sorter

Signed-off-by: zeminzhou <zhouzemin@pingcap.com>

* remove debug log

Signed-off-by: zeminzhou <zhouzemin@pingcap.com>

* add metric for the input event of puller

Signed-off-by: zeminzhou <zhouzemin@pingcap.com>

* remove puller input metric

Signed-off-by: zeminzhou <zhouzemin@pingcap.com>

* add back

Signed-off-by: zeminzhou <zhouzemin@pingcap.com>

Signed-off-by: zeminzhou <zhouzemin@pingcap.com>
Signed-off-by: pingyu <yuping@pingcap.com>
pingyu added a commit that referenced this pull request Oct 9, 2022
* [fix #246] fix unified sorter (#247)

* fix unified sorter

Signed-off-by: zeminzhou <zhouzemin@pingcap.com>

* remove debug log

Signed-off-by: zeminzhou <zhouzemin@pingcap.com>

* add metric for the input event of puller

Signed-off-by: zeminzhou <zhouzemin@pingcap.com>

* remove puller input metric

Signed-off-by: zeminzhou <zhouzemin@pingcap.com>

* add back

Signed-off-by: zeminzhou <zhouzemin@pingcap.com>

Signed-off-by: zeminzhou <zhouzemin@pingcap.com>
Signed-off-by: pingyu <yuping@pingcap.com>

* fix keyspan sink metrics (#249)

Signed-off-by: pingyu <yuping@pingcap.com>

Signed-off-by: pingyu <yuping@pingcap.com>

* fix gh action

Signed-off-by: pingyu <yuping@pingcap.com>

Signed-off-by: zeminzhou <zhouzemin@pingcap.com>
Signed-off-by: pingyu <yuping@pingcap.com>
Co-authored-by: zzm <zhouzemin@pingcap.com>
@zeminzhou zeminzhou deleted the fix-sort branch October 17, 2022 13:19
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: unified sorting is not working properly
3 participants