set default incremental checkpoint interval to 5m#25432
Conversation
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
(cherry picked from commit 0ba8b1b)
XuPeng-SH
left a comment
There was a problem hiding this comment.
Requesting changes after reviewing latest head 0b98efc2a39ad1d080796fcd2305f76a0bacd189 against 4.1-dev.
The core Go default change is internally consistent, but the behavior is not closed end-to-end yet.
- The launch/embed configs still explicitly override the new default with
incremental-interval = "180s".
This PR changes the zero-value default to 5 minutes, but all of these shipped startup templates still set an explicit 3 minute interval, so deployments using them will not see the new default:
pkg/embed/template.goetc/launch-dynamic-with-proxy/tn.tomletc/launch-dynamic-cn/tn.tomletc/launch-multi-cn/tn.tomletc/launch-with-proxy/tn.tomletc/launch-minio-local/tn.tomletc/launch-tae-compose/config/tn.toml
If the intended release behavior is 5m, those explicit configs need to be updated or removed so the Go default can apply. Otherwise this only changes code paths where the field is omitted, while the common packaged/dev launch configs keep the old value.
- The arena pool drain timer still assumes the old 1m checkpoint interval.
pkg/objectio/arena_pool.go documents arenaDrainDelay as 2x the default incremental checkpoint interval so the timer is reset by steady-state checkpointing and pools stay warm. The value is still 2 * time.Minute; after this PR, the default checkpoint interval becomes 5m, so the drain timer can fire between normal checkpoints and empty the arena free lists during active operation. That is a performance regression risk and contradicts the documented invariant. Please update this delay/design alongside the new default, or justify why the old 2m drain is still correct after changing checkpoint cadence.
- Please add focused regression coverage for the default value propagation.
At minimum, cover the three default-entry paths changed here:
tnservice.Config.Validate/SetDefaultValueoptions.Options.FillDefaultscheckpoint.CheckpointCfg.FillDefaults
That would catch drift between TN config, TAE options, and checkpoint runtime defaults.
Local verification I ran:
go test -count=1 -timeout 240s ./pkg/vm/engine/tae/options ./pkg/vm/engine/tae/db/checkpoint ./pkg/tnservicego test -count=1 -timeout 240s ./pkg/vm/engine/tae/options ./pkg/vm/engine/tae/db/checkpoint ./pkg/tnservice -run '^$'go vet ./pkg/vm/engine/tae/options ./pkg/vm/engine/tae/db/checkpoint ./pkg/tnservice
Those passed locally, but the end-to-end config/behavior gaps above should be fixed before merge.
XuPeng-SH
left a comment
There was a problem hiding this comment.
Requesting changes for one remaining inconsistency:
etc/docker-multi-cn-local-disk/generate-config.sh:442still emitsincremental-interval = "60s".- The rest of the PR consistently moves the default incremental checkpoint interval to 5 minutes (
pkg/vm/engine/tae/options/types.go,pkg/tnservice/cfg.go,pkg/vm/engine/tae/db/checkpoint/cfg.go, the touchedetc/launch-*TN configs, andpkg/embed/template.go), so this local docker config generation path is still on the old value. - If this divergence is intentional, it should be called out explicitly; otherwise it leaves one non-CI startup path silently outside the new default and defeats the stated goal of the PR.
Suggested fix: update the generator to 300s, or route it through the same shared default/template source as the other launcher configs.
What type of PR is this?
Which issue(s) this PR fixes:
issue #24900
What this PR does / why we need it:
set default incremental checkpoint interval to 5m