Skip to content

Default report frequencies and reject values below 100ms - #466

Open
driv3r wants to merge 2 commits into
mainfrom
fix-report-frequency-default
Open

driv3r wants to merge 2 commits into
mainfrom
fix-report-frequency-default

Conversation

@driv3r

@driv3r driv3r commented Oct 14, 2026

Copy link
Copy Markdown
Contributor

Fixes #214. Supersedes #215.

Problem

ProgressReportFrequency and StateReportFrequency have no default and no validation. The reporting loops wait on time.After(time.Duration(freq) * time.Millisecond), so with a callback URI set and the frequency left at 0 (or set negative) the loop fires immediately, forever. Each iteration builds a full Progress (or serializes the whole state) and POSTs it synchronously.

Smoke run with the real ghostferry-copydb binary on a 3-row table, ProgressCallback set and no frequency:

binary config progress POSTs result
main frequency unset 1053 in 1.5 s completes
this PR frequency unset 2 (cutover + done) completes
this PR ProgressReportFrequency: 500 4 completes
this PR ProgressReportFrequency: -1 0 failed to validate config: ProgressReportFrequency must be at least 100 milliseconds, got -1

Fix

In Config.ValidateConfig:

  • 0 defaults to 60000 ms, matching how other optional numeric settings are defaulted (BinlogEventBatchSize, DBReadRetries, ...).
  • Values below 100 ms are rejected when the matching callback URI is set. That covers negative values and "meant seconds, wrote milliseconds" typos.

#215 clamped to a 1-minute minimum inside the loops instead. That would silently override the 500 ms the integration tests use and the 1500 ms the benchmark uses. Rejecting at validation reports the misconfiguration at startup instead.

All in-repo entry points (copydb, sharding, integration ferry, test helpers) call ValidateConfig.

Repro

The first commit adds only the tests:

  • TestConfig/TestReportFrequenciesDefaultWhenCallbacksAreSet: before the fix the frequency stayed 0.
  • TestConfig/TestReportFrequenciesBelowMinimumAreRejected: before the fix there was no error for -1 or 50.

Verification (local, MySQL 8.0.32)

  • go test ./copydb/test ./sharding/test and go test ./test/go -run 'TestConfig|TestLogger': pass.
  • test/integration/callbacks_test.rb: 3 tests, 0 failures.
  • The smoke table above, run against the local containers with main and this branch.

With a progress or state callback configured and the frequency left at
0 (or set negative), the reporting loop waits on time.After(0) and posts
to the callback as fast as it can.
ProgressReportFrequency and StateReportFrequency now default to 60000ms
like other optional settings, and ValidateConfig rejects values below
100ms when the matching callback is configured.

Fixes #214.
@driv3r
driv3r requested review from grodowski and mtaner October 19, 2026 16:44

This branch has not been deployed

No deployments
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.

Setting Config.ProgressReportFrequency to a small or negative number would be bad

1 participant