Bug Report: cannot enable always-on heartbeats if on demand heartbeats are configured

[deepthi]

Overview of the Issue

I tried to enable heartbeats on a shard where all tablets had --heartbeat_on_demand_duration=10s. Once all tablets had been rolled with the new flag --heartbeat_enable=true it was observed that no heartbeats were actually being written into the _vt.heartbeat table.

Workaround: set --heartbeat_on_demand_duration=0s.

Reproduction Steps

Run the operator example, with vttablet extraFlags --heartbeat_enable=true and --heartbeat_on_demand_duration=10s

Binary Version

v19

Operating System and Environment details

any

Log Fragments

[deepthi]

We should handle the config.ReplicationTracker.Mode == tabletenv.Heartbeat case before config.ReplicationTracker.HeartbeatOnDemand > 0. See https://github.com/vitessio/vitess/blob/main/go/vt/vttablet/tabletserver/repltracker/writer.go#L101-L110

[shlomi-noach]

See also:

• This comment
• This discussion

Right now the flags are working as documented and as designed. If we now switch the "override" priority such that --heartbeat_enable overrides --heartbeat_on_demand_duration, this will break the expected behavior, and setups that expect heartbeat to be on-demand only, will suddenly see a surge of heartbeats.

It's a bit of a mess and if we want to break this behavior we might need a two-version rollout? (1st rollout is to announce, warn; 2nd rollout is to possibly make the flags mutually exclusive, force the new behavior?).

From #17838 (comment):

The default for heartbeats is 1s, but you might want to make them more frequent when they are on-demand without affecting the normal frequency. It was probably not a good idea to use the same frequency for both kinds of heartbeats.

That doesn't play well with the way the heartbeat writer is designed. Not impossible. If we want to do this, we may wish to rewrite the heartbeat writer mechanism, something I've avoided thus far.