SEZ9 commented on PR #12789:
URL: https://github.com/apache/seatunnel/pull/12789#issuecomment-6282522365
Thanks @dybyte — that is the sharpest question anyone has asked about this
change, and you are right that there is a mismatch in it.
My whole motivation is time-based ("32 entries cover about 5 minutes at a
10s interval"), and what I added is a count. Since `checkpoint.interval` is per
job while this option is cluster-wide, one value covers a different stretch of
history on every job in the cluster. A count cannot express "keep 20 minutes",
which is what an operator actually wants.
I have taken your suggestion of landing the count first and leaving
time-based retention as a follow-up, and @DanielLeens asked for the limitation
to be stated rather than left implicit. Both the option description and the two
documentation pages now say it outright: it is a count, not a duration; how
much time it covers depends on each job's own `checkpoint.interval`; this
option is cluster-wide.
Two reasons I think count-first is the right first step rather than a
shortcut:
- **It is the shape the retention code already has.**
`PipelineCheckpointOverview.addHistory(entry, maxHistory)` trims a deque by
size. Time-based retention means evicting by `triggerTimestamp`, which is a
different eviction rule in a structure that is re-serialised on every
checkpoint event — a real change rather than a parameter.
- **The default is unchanged (32),** so this PR cannot regress an existing
cluster, which makes it cheap to land and cheap to supersede.
What I would *not* want is for the count option to make time-based retention
awkward later. I do not think it does: a future `monitor-history-retention`
could coexist (evict when either bound is hit) or deprecate this one, and
because the count has a documented default rather than being load-bearing in
any API, retiring it would be a deprecation cycle rather than a breaking change.
Also addressed in the follow-up commit, from @DanielLeens's review:
- the setter is now bounded to 1..4096. The upper bound matters more than I
first wrote: `HazelcastCheckpointOverviewStateStore.updateOverview` rewrites
the whole `CheckpointOverview` through `IMap.compute` on every checkpoint
event, and the entry listener is registered `includeValue=true`, so an
oversized window multiplies master CPU, replication and listener traffic — not
just heap. My original docs said "memory per pipeline", which understated it;
that wording is corrected.
- `monitor-history-size` is added next to `min-pause` in
`hybrid-cluster-deployment.md` (en and zh).
- tests now pin the default from a config with no `checkpoint` block, the
bounds at 0, -1, 1, max and max+1, and a new `PipelineCheckpointOverviewTest`
pins the trimming the option drives, including that the retained entries are
the newest ones.
On the one hop that is still not pinned by a test: nothing asserts that
`SeaTunnelServer.startMaster()` passes the configured value rather than a
literal, so a regression there would not turn a test red. Unit-testing it needs
a running node, and I did not want to add an IT for one argument. The config
path and the trimming behaviour are both covered now, so the gap is that single
line, and I would rather say so than imply otherwise.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
