-
Notifications
You must be signed in to change notification settings - Fork 286
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
cordinator(ticdc): Fix Puller Resolved TS Lag Calculation and Deprecate current_ts Field in Stats #11624
cordinator(ticdc): Fix Puller Resolved TS Lag Calculation and Deprecate current_ts Field in Stats #11624
Conversation
…log.Panic` to `log.Error` to prevent disruption to other changefeeds
…low-table-puller-resolved-ts-lag-negative-year
…cause of uninitialized `CurrentTs`
…us.CurrentTs` which defined in table.pb.go
….CurrentTs` which defined in table.pb.go
Codecov ReportAttention: Patch coverage is
Additional details and impacted files
Flags with carried forward coverage won't be shown. Click here to find out more. @@ Coverage Diff @@
## master #11624 +/- ##
================================================
- Coverage 57.4909% 55.3542% -2.1367%
================================================
Files 851 1001 +150
Lines 126486 136349 +9863
================================================
+ Hits 72718 75475 +2757
- Misses 48377 55386 +7009
- Partials 5391 5488 +97 |
Co-authored-by: nhsmw <[email protected]>
/retest |
/retest |
1 similar comment
/retest |
/test cdc-integration-storage-test |
/test cdc-integration-mysql-test |
/retest |
[LGTM Timeline notifier]Timeline:
|
In response to a cherrypick label: new pull request created to branch |
Signed-off-by: ti-chi-bot <[email protected]>
Signed-off-by: ti-chi-bot <[email protected]>
In response to a cherrypick label: new pull request created to branch |
In response to a cherrypick label: new pull request created to branch |
…te current_ts Field in Stats (pingcap#11624) close pingcap#11561
In response to a cherrypick label: new pull request created to branch |
…te current_ts Field in Stats (pingcap#11624) close pingcap#11561
What problem does this PR solve?
Issue Number: close #11561
What is changed and how it works?
Summary:
This PR addresses a critical issue in the Puller Resolved TS Lag metric, where the lag was incorrectly displayed as -55 years. The root cause of the problem was the improper initialization of the
CurrentTs
field in theStats
structure withintable.pb.go
. This led to a situation whereCurrentTs
remained zero whileResolvedTs
was correctly populated, resulting in a negative lag calculation that erroneously reflected a time difference of 55 years (from 1970 to the current year).Detailed Explanation:
Issue Background:
CurrentTs
andResolvedTs
from theStats
structure.NewReplicationSet
function did not correctly initializeCurrentTs
, leading to incorrect lag values when metrics were collected.Design Intent of
current_ts
:current_ts
field was initially designed to record the time at the processor side, which would then be used by the coordinator for lag calculation. This design was meant to avoid including the transmission delay and processing time on the coordinator side, as these could introduce inaccuracies in the lag calculation.current_ts
field may introduce a slight inaccuracy in the lag calculation, as the "current time" will now be fetched directly by the coordinator rather than from the processor. This could result in a minor error due to transmission delays and processing time. However, this inaccuracy is negligible for practical purposes, as the delay is typically within milliseconds and does not significantly impact the metric, which is measured in seconds.pb
) structure, it added extra overhead to every network transmission by carrying an additional value. It also introduced more complexity into the code, requiring maintenance of this field, which was part of the issue that led to the incorrect metric calculations in this case.Root Cause:
CurrentTs
from theStats
structure was flawed because this timestamp was not reliably up-to-date.CurrentTs
stored in theStats
structure.Solution Implemented:
CurrentTs
field within the codebase, particularly in metric calculations, have been updated to reflect this change.Deprecation of
current_ts
Field:current_ts
field in theStats
structure, defined incdc/processor/tablepb/table.proto
, has been marked as deprecated.Stats
structure is used for data transmission between the Coordinator and Processor, wherecurrent_ts
was unnecessarily included in the state for each Table Span.current_ts
within the project, ensuring that the Coordinator and Processor no longer rely on this outdated field.Impact:
This PR ensures accurate lag calculation in the Puller Resolved TS Lag metric and removes the outdated
current_ts
field, improving both performance and maintainability by reducing unnecessary transmission overhead and field maintenance. While this change may introduce a slight inaccuracy in the lag calculation due to the use of coordinator-side timestamps, this impact is minimal and well within acceptable thresholds.Check List
Tests
Questions
Will it cause performance regression or break compatibility?
No. The deprecation of the current_ts field in the Stats structure is backward-compatible because the field is not actively used elsewhere in the project beyond the affected metrics. The existing field remains in the protocol buffer definition but is marked as deprecated, so it won’t break compatibility with older versions that still use the field.
Do you need to update user documentation, design documentation or monitoring documentation?
No.
Release note