cauchy1988 opened a new pull request, #2411:
URL: https://github.com/apache/incubator-pegasus/pull/2411
## Problem (#1564)
When `manual_compact.periodic.trigger_time` is set to a time of day that has
already passed (e.g. `1:00` while it is now `14:00`), periodic manual
compaction fires **immediately**. This is catastrophic for **duplication
full-app duplicate**: the backup cluster creates a brand-new app and copies
the primary's manual-compact envs, so the backup's RocksDB performs
compaction before it is fully initialized → undefined behavior → **all nodes
in the backup cluster crash**.
Closes #1564.
## Root cause
Each partition uses `_manual_compact_last_finish_time_ms`, initialized to
`0`,
as the **"never compacted"** sentinel. But `check_periodic_compact()` treats
that `0` as a distant-past lower bound:
```cpp
// before
if (_manual_compact_last_finish_time_ms.load() < t_ms && t_ms < now)
return true;
// ^ 0 < t_ms is always true => reduces to "trigger time already
passed"
```
The new-DB path (which is exactly what a duplication backup app is — a
freshly
created app) never calls `init_last_finish_time_ms`, so `last_finish` stays
`0`.
`check_manual_compact_state()` also sees `== 0` and lets it through. So an
already-passed trigger retro-fires at the very start of the backup app's life
(the check even runs from `update_app_envs_before_open_db`, i.e. before the
DB
is fully open), before RocksDB is ready.
## Fix
Keep the `last_finish == 0` "never compacted" sentinel intact, and use the
**service start time** as the trigger-time lower bound only when
`last_finish == 0`:
```cpp
// after
uint64_t last_finish = _manual_compact_last_finish_time_ms.load();
uint64_t lower_bound = last_finish == 0 ? _app_start_time_ms.load() :
last_finish;
for (auto t : trigger_time) {
auto t_ms = t * 1000;
if (lower_bound < t_ms && t_ms < now)
return true;
}
```
`_app_start_time_ms` is a new member fixed at construction time
(`dsn_now_ms()`).
Why this design:
- **Preserves the `0` sentinel.** `check_manual_compact_state()`'s
min-interval
branch and `query_compact_state()`'s display both rely on `last_finish ==
0`
meaning "never compacted". We do not touch
`_manual_compact_last_finish_time_ms`.
- **One mechanism, both DB paths.** The floor keys off `last_finish == 0` at
check time, so it covers both the new-DB path (no `init` call) and the
existing-but-never-compacted path (`init(0)`). No change to
`init_last_finish_time_ms` is needed.
- **No deadlock.** The floor is the fixed start time, not a dynamic `now`, so
the first periodic trigger after startup (e.g. start 14:00, trigger 18:00)
still fires normally once `now` passes it.
- **Timing-safe.** The periodic check only runs after the app is open, so a
trigger within `(start, now)` fires on an already-initialized RocksDB; only
triggers that already passed *before* startup (the retro-fire) are
suppressed.
Apps that have already been compacted keep using `last_finish`, so the
existing "catch up on a missed daily trigger after a restart" behavior is
unchanged.
## Test
New regression test `check_periodic_compact_never_compacted` mocks the
never-compacted state (`last_finish == 0`, app start `14:00`):
- trigger `1:00` (already passed before startup) → must **not** fire
- trigger `18:00` not yet reached (`now == 14:00`) → must **not** fire
- advance `now` to `19:00` → `18:00` now lies in `(start, now)` → **fires**
(proves no deadlock)
Forward run: all `manual_compact_service_test.*` pass (14 cases).
Reverse check (temporarily neutering the floor): the new case fails as
expected, confirming the test actually catches the bug.
## Scope / notes
- `check_once_compact()` has analogous behavior when `last_finish == 0`, but
#1564 is specifically about `periodic.trigger_time`; the once-trigger is
out
of scope here.
- Residual narrow window: if a backup app starts *before* a trigger time and
the check runs after that time passes but before duplication is ready, it
could still fire. Fully closing that requires duplication-readiness
awareness, which lives in the replica layer (out of scope for this fix).
This PR fixes the primary scenario — crash on startup.
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
