syncer(dm): initialize binlog metrics from checkpoint - #12767
Conversation
ti-chi-bot
Bot
added
first-time-contributor
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
|
ti-chi-bot
Bot
added
the
size/L
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthrough
ChangesBinlog metric initialization
Estimated code review effort: 3 (Moderate) | ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.12.2)Error: can't load config: unsupported version of the configuration: "" See https://golangci-lint.run/docs/product/migration-guide for migration instructions Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Fresh tasks can replace the persisted checkpoint with a start-time or metadata location in Run. Refresh the binlog gauges after GTID adjustment so they reflect the final start point before the first event.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
dm/syncer/syncer_metrics_test.go (1)
42-65: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd coverage for malformed non-empty binlog filenames.
The cases cover empty and GTID-only checkpoints, but not a checkpoint with a non-empty, unparsable filename. Add one to verify the file gauge becomes
NaNwhile the position gauge preserves the checkpoint position.Proposed test case
{ name: "fresh checkpoint with empty binlog filename", checkpoint: binlog.MustZeroLocation(mysql.MySQLFlavor), expectedFile: math.NaN(), expectedPos: float64(binlog.MinPosition.Pos), }, + { + name: "checkpoint with malformed binlog filename", + checkpoint: binlog.NewLocation(mysql.Position{ + Name: "not-a-binlog-name", + Pos: 123, + }, nil), + expectedFile: math.NaN(), + expectedPos: 123, + },As per coding guidelines, DM fixes and features should include unit tests.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@dm/syncer/syncer_metrics_test.go` around lines 42 - 65, Add a test case alongside the existing checkpoint cases in the syncer metrics test table using a non-empty, malformed binlog filename and a known position. Assert that the file gauge result is math.NaN() while the position gauge retains the checkpoint’s position, reusing the existing checkpoint construction and expected-value conventions.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@dm/syncer/syncer_metrics_test.go`:
- Around line 42-65: Add a test case alongside the existing checkpoint cases in
the syncer metrics test table using a non-empty, malformed binlog filename and a
known position. Assert that the file gauge result is math.NaN() while the
position gauge retains the checkpoint’s position, reusing the existing
checkpoint construction and expected-value conventions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 5ce2776c-08f2-4670-b8d6-77ba7def8487
📒 Files selected for processing (2)
dm/syncer/syncer.godm/syncer/syncer_metrics_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- dm/syncer/syncer.go
Cover the non-empty filename parse failure path and initialize the test context required by its warning log.
Fix it |
|
/retest-required |
D3Hunter
left a comment
D3Hunter
left a comment
There was a problem hiding this comment.
Summary
- Total findings: 2
- Inline comments: 2
- Summary-only findings (no inline anchor): 0
Findings (highest risk first)
🟡 [Minor] (2)
- Initializer name contradicts the helper's refresh behavior (
dm/syncer/syncer.go:554) - Checkpoint-to-metric lifecycle wiring is not covered (
dm/syncer/syncer_metrics_test.go:30; dm/syncer/syncer.go:548; dm/syncer/syncer.go:1860)
| return nil | ||
| } | ||
|
|
||
| func (s *Syncer) initSyncerBinlogMetrics(checkpoint binlog.Location) { |
There was a problem hiding this comment.
🟡 [Minor] Initializer name contradicts the helper's refresh behavior
Why
The helper is not limited to initialization: Run deliberately calls it again after the effective global checkpoint may change. Naming this repeated gauge update initSyncerBinlogMetrics obscures that it is safe and intended to overwrite existing metric values.
Scope
dm/syncer/syncer.go:554
Risk if unchanged
Future callers may treat the helper as a one-time setup operation, miss required refreshes after checkpoint changes, or add initialization-only work that is unsafe on the second call.
Evidence
The new comment at dm/syncer/syncer.go:1857 explicitly says to "Refresh" the metrics, but line 1860 invokes initSyncerBinlogMetrics; the same method is first called from Init at line 548, and the test name TestInitSyncerBinlogMetrics reinforces the one-time interpretation.
Change request
Prefer setSyncerBinlogMetrics or updateSyncerBinlogMetrics; the current name is confusing. Rename the helper and its test so both initialization and later refresh call sites describe the same repeated update semantics.
| "github.com/stretchr/testify/require" | ||
| ) | ||
|
|
||
| func TestInitSyncerBinlogMetrics(t *testing.T) { |
There was a problem hiding this comment.
🟡 [Minor] Checkpoint-to-metric lifecycle wiring is not covered
Why
The changed behavior depends on publishing the loaded checkpoint in Init and then republishing the final start checkpoint after metadata, start-time, and GTID adjustments in Run, but the new test validates only the conversion helper in isolation.
Scope
dm/syncer/syncer_metrics_test.go:30; dm/syncer/syncer.go:548; dm/syncer/syncer.go:1860
Risk if unchanged
A missing or misplaced lifecycle call can leave an idle or newly started task reporting zero, NaN, or a stale persisted checkpoint until its first binlog event, while this regression test still passes and the intended observability fix is lost.
Evidence
TestInitSyncerBinlogMetrics constructs standalone gauges and invokes s.initSyncerBinlogMetrics(tc.checkpoint) directly; it never exercises Syncer.Init, LoadMeta, setGlobalPointByTime, adjustGlobalPointGTID, or either production call site.
Change request
Please add an integration test for this path that asserts the exposed gauges after Init loads a persisted checkpoint and after Run selects a different metadata, start-time, or GTID-adjusted checkpoint before any binlog event is consumed.
D3Hunter
left a comment
D3Hunter
left a comment
There was a problem hiding this comment.
Additional review finding
🟠 [Major] The original alerting issue is only partially fixed
The production change correctly restores the syncer file and position gauges from the persisted checkpoint. It changes the reported example from 347650 - 0 = 347650 to the real file gap, 347650 - 346652 = 998.
However, dm/metrics/alertmanager/dm_worker.rules.yml:128 still evaluates every exported syncer metric without checking task state. Because 998 > 1, an intentionally paused task can still trigger DM_binlog_file_gap_between_master_syncer after this PR. This fixes the misleading zero-based inflation, but it does not satisfy issue #12765's first expected behavior: paused or stopped tasks should not trigger the gap alert.
Please treat this as a partial fix and keep #12765 open until the alert is restricted to running tasks, for example by filtering or joining with dm_worker_task_state == 2. The join labels should preserve task and source identity so multi-source tasks do not produce ambiguous matches.
The checkpoint-to-metric lifecycle test gap is already covered by the existing inline comment at dm/syncer/syncer_metrics_test.go:30, so I did not duplicate it.
filtering |
Rename the checkpoint metric helper to reflect its update semantics and add lifecycle coverage for Init and Run call sites.
ti-chi-bot
Bot
added
size/XL
|
@expxiaoli: The following test failed, say
Full PR test history. Your PR dashboard. DetailsInstructions 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. |
2 participants
What problem does this PR solve?
Issue Number: ref #12765
When a DM task is loaded on another worker but remains stopped or paused, the syncer is initialized and its metrics are registered, but the replication loop does not run. As a result, the syncer binlog file and position gauges keep their default values even when a valid persisted checkpoint exists.
The master binlog gauge continues to be updated by status polling, so the default syncer file number
0can produce a significantly inflated master-to-syncer binlog gap.This PR addresses the checkpoint metric initialization part of the issue.
What is changed and how it works?
NaNwhen the checkpoint has no valid binlog filename, such as a fresh or GTID-only checkpoint, instead of exposing the default value0as a valid file number.Check List
Tests
Questions
Will it cause performance regression or break compatibility?
No performance regression or compatibility break is expected. The gauges are initialized once during syncer initialization, and no replication or checkpoint behavior is changed. For checkpoints without a binlog filename, the file gauge now reports
NaNuntil a valid file position is available instead of reporting a misleading zero.Do you need to update user documentation, design documentation or monitoring documentation?
No. Metric names and labels are unchanged; this PR only corrects their initialization values.
Release note
Summary by CodeRabbit