Skip to content

[S2.9 / C1] Config plumbing: master-switch + active-sweeper + interval#29

Merged
ikolomi merged 3 commits into
unstablefrom
ikolomi/s2-config-plumbing
Jun 14, 2026
Merged

[S2.9 / C1] Config plumbing: master-switch + active-sweeper + interval#29
ikolomi merged 3 commits into
unstablefrom
ikolomi/s2-config-plumbing

Conversation

@ikolomi

@ikolomi ikolomi commented Jun 14, 2026

Copy link
Copy Markdown
Owner

Summary

PR-C1 of 3 in the implementation of the declarative master-switch model from PR #28 (PR-B). Replaces the bool compression-enabled config with the 3-state enum compression-master-switch and adds the active-sweeper configs. C1 is config plumbing only — the transient-view drain mode dispatch lands in C2, the sweeper engine + COMPRESSION SWEEP FORCE command land in C3.

What changes

New configs (R2.1.1 – R2.1.3)

Name Type Default Replaces / Adds
compression-master-switch enum: off / compression / decompression off replaces compression-enabled bool
compression-active-sweeper enum: disabled / enabled disabled new
compression-active-sweeper-interval int (seconds) 0 new

Plus matching INFO fields: compression_master_switch, compression_active_sweeper, compression_active_sweeper_interval. The legacy compression_enabled:0 INFO field is removed.

Two new enum constant blocks in server.h:

#define COMPRESSION_MASTER_OFF 0
#define COMPRESSION_MASTER_COMPRESSION 1
#define COMPRESSION_MASTER_DECOMPRESSION 2

#define COMPRESSION_ACTIVE_SWEEPER_DISABLED 0
#define COMPRESSION_ACTIVE_SWEEPER_ENABLED 1

Apply hooks (R2.1.5 – R2.1.6)

applyCompressionMasterSwitch implements the auto-retire policy: on any transition INTO decompression (from compression or from off), the active dict is retired via compressionRegistryRetire(active_id) so it can drain. All other transitions leave the registry untouched. The hook logs the transition at LL_NOTICE.

applyCompressionActiveSweeper is minimal in C1 — logs the transition. Engine wiring is C3.

maybeWarnNonFunctional is called from both apply hooks plus applyCompressionThreads (via the new compressionAfterThreadsApplied shim in config.c). Emits LL_WARNING once on transition INTO master=compression + threads=0 (R2.1.6).

Static cache initialization

The apply hooks track prior values via file-static caches. compressionInit() calls syncApplyHookCaches() AFTER boot config is applied, which fixes two boot-time bugs:

  1. The non-functional warning (master=compression + threads=0 set at boot) didn't fire because apply hooks don't run during boot config load. Now the sync function explicitly calls maybeWarnNonFunctional() once at init.
  2. An operator booting with master=compression and later setting master=decompression would have seen the apply hook log a misleading "off -> decompression" transition (because the static was initialized to the default, not the boot value). Now the static caches reflect the boot values, so the first runtime transition logs correctly.

Verified manually with:

./src/valkey-server --compression-master-switch compression --compression-threads 0
# → boot-time LL_WARNING fires
./src/valkey-cli config set compression-master-switch decompression
# → logs "compression -> decompression" (not "off -> decompression")

Stale-reference sweep (PR-B vocabulary update)

File Change
src/compression.c 4 comment updates + 2 production checks (compressionIsEligible, compressionEnqueueModified) gated on master == COMPRESSION_MASTER_COMPRESSION. INFO renderer rewritten.
src/compression_workers.c 1 comment line (now references R2.1.7 third state, not R2.1.5).
src/compression_train.c 1 production check + comment. Cron gated on master == compression (drain mode and off both block training).
src/compression_registry.{h,c} 2 comments + 1 log message — replaced COMPRESSION SWEEP direction=decompress with the new operator surface (master=decompression + active-sweeper=enabled).
src/compression.h Removed the unused compressionToggle() declaration (Phase-0 stub, never called); replaced with the apply-hook declarations + compressionAfterThreadsApplied shim.

COMPRESSION ENABLE / COMPRESSION DISABLE aliases are referenced exactly once — in a doc comment in compression.h explicitly noting they are NOT part of v1's surface.

Test updates

  • src/unit/test_compression_eligibility.cpp — replaced server.compression_enabled with server.compression_master_switch using the new enum constants. Added MasterSwitchDecompressionBlocksEligibility verifying the predicate also rejects in master=decompression (drain mode).
  • src/unit/test_compression_train.cpp — same conversion.
  • src/unit/test_compression_workers.cpp — 1 comment update.
  • tests/unit/type/compression.tcl — rewrote tests referencing compression-enabled. Added two new tests verifying the enum accepts each documented value and rejects out-of-set values for both compression-master-switch and compression-active-sweeper. Updated the knob-default-count test (now 18 knobs including the 3 new ones).

What's NOT in this PR (later C-series)

  • C2: transient-view two-mode drain (R2.5.7). compressionMaterializeTransientView and compressionBeforeSleep need a master == decompression branch that permanent-decompresses instead of restoring.
  • C3: sweeper engine + COMPRESSION SWEEP FORCE command. Replaces PR [S2.9 + S2.10] Sweep state machine + COMPRESSION SWEEP command #26's content (which will be closed when C3 lands).

Verified locally

make -j2 -C src SERVER_CFLAGS=-Werror                       # clean (BUILD_ZSTD=yes default)
make -C src distclean
make -j2 -C src SERVER_CFLAGS=-Werror BUILD_ZSTD=no         # clean
./runtest --single unit/type/compression                    # 13/13 pass

Boot/runtime smoke:

  • ./src/valkey-server --compression-master-switch compression --compression-threads 0 emits the boot-time warning.
  • CONFIG SET compression-master-switch compression -> decompression -> off logs each transition correctly with the right "from" values.
  • INFO compression and COMPRESSION STATUS render the new fields and stay in lockstep (the meta-test in compression.tcl enforces this).

Not verified locally (CI gates)

  • gtest unit tests (libgtest-dev not installed in this dev environment).
  • clang-format-18.

Diff stat

src/compression.c                         | 190 ++++++++++++++++++++++++++----
src/compression.h                         |  30 +++--
src/compression_registry.c                |   4 +-
src/compression_registry.h                |  11 +-
src/compression_train.c                   |   6 +-
src/compression_workers.c                 |   4 +-
src/config.c                              |  24 +++-
src/server.h                              |  25 +++-
src/unit/test_compression_eligibility.cpp |  20 +++-
src/unit/test_compression_train.cpp       |   4 +-
src/unit/test_compression_workers.cpp     |   2 +-
tests/unit/type/compression.tcl           |  83 ++++++++-----
12 files changed, 327 insertions(+), 76 deletions(-)

Implements the declarative master-switch model from PR-B (design doc):
replaces the bool `compression-enabled` config with the 3-state enum
`compression-master-switch` (compression / decompression / off, default
off), adds the `compression-active-sweeper` enum (enabled / disabled,
default disabled) and `compression-active-sweeper-interval` time config
(default 0 = no periodic re-runs). Matching INFO fields. Apply hooks
implementing the R2.1.5 transition rules and the R2.1.6 non-functional
state warning. C1 stops at config plumbing; the transient-view drain
mode dispatch is C2, the sweeper engine + COMPRESSION SWEEP FORCE
command are C3.

Master-switch transition table (R2.1.5):
  compression -> decompression : auto-retire active dict (drain mode)
  compression -> off           : NO retire (state frozen)
  off         -> decompression : auto-retire active dict if any
  decompression -> off         : no action (already retired)
  off -> compression           : no action; existing dict reusable
  decompression -> compression : no action; falls into R2.1.7 third state

Apply-hook implementation notes:

- The hooks track the prior value via file-static caches (one for
  master, one for active-sweeper, one shared for the warning detector
  + a parallel for compression-threads). compressionInit() calls
  syncApplyHookCaches() AFTER boot config is applied, which fixes two
  bugs:
    1. Boot-time non-functional warning (master=compression+threads=0
       set at boot) didn't fire because apply hooks don't run during
       boot config load.
    2. An operator booting with master=compression and later setting
       master=decompression would have seen the apply hook log a
       misleading "off -> decompression" transition (because the
       static was initialized to the default, not the boot value).

- applyCompressionThreads in config.c calls compressionAfterThreadsApplied
  to keep the warning detector's threads cache in sync.

- maybeWarnNonFunctional emits LL_WARNING once on transition INTO
  master=compression+threads=0; subsequent CONFIG SETs that don't
  leave the state are no-ops.

Stale-reference sweep (PR-B vocabulary update):

- src/compression.c: 4 comments and 2 production checks updated from
  "compression-enabled" / server.compression_enabled to master-switch
  language. INFO field rendering switched from compression_enabled:0
  to compression_master_switch:<name> + new compression_active_sweeper
  + compression_active_sweeper_interval fields.
- src/compression_workers.c: 1 comment line.
- src/compression_train.c: gated cron entry on master==compression
  instead of !enabled bool.
- src/compression_registry.h: 2 comments referencing
  "COMPRESSION SWEEP direction=decompress" updated to the new operator
  surface (master=decompression + active-sweeper=enabled).
- src/compression_registry.c: log message at dict-cap-reached.
- src/compression.h: removed unused compressionToggle() declaration
  (was a Phase-0 stub, never called); replaced with the apply-hook
  declarations.

Test updates:

- src/unit/test_compression_eligibility.cpp: replaced
  server.compression_enabled with server.compression_master_switch
  using the new enum constants. Added a new test case
  MasterSwitchDecompressionBlocksEligibility verifying that the
  predicate also rejects in master=decompression (drain mode).
- src/unit/test_compression_train.cpp: same conversion.
- src/unit/test_compression_workers.cpp: 1 comment update.
- tests/unit/type/compression.tcl: rewrote 5 tests that referenced
  compression-enabled to use compression-master-switch. Added two
  new tests verifying the enum accepts each documented value plus
  rejects out-of-set values for both master-switch and active-sweeper.
  Updated the 18-knob defaults check (was 16 knobs; the rename from
  1 bool to 1 enum + 2 new sweeper configs increases primary-knob
  count from 5 to 6, advanced unchanged at 11, hence 17... wait,
  the test count is 18 because compression-active-sweeper-interval
  is a separate config registration. Confirmed by the test passing.)

Verified:

  make -j2 -C src SERVER_CFLAGS=-Werror   # clean (BUILD_ZSTD=yes)
  make -C src distclean
  make -j2 -C src SERVER_CFLAGS=-Werror BUILD_ZSTD=no   # clean
  ./runtest --single unit/type/compression  # 13/13 pass

  Boot/runtime smoke:
   - --compression-master-switch compression --compression-threads 0
     emits boot-time LL_WARNING.
   - CONFIG SET master compression -> decompression -> off logs the
     transitions correctly with the right "from" values.
   - INFO compression and COMPRESSION STATUS render the new fields.

Not verified locally:

  - gtest unit tests (libgtest-dev not installed). CI validates.
  - clang-format-18. CI validates.
ikolomi added 2 commits June 14, 2026 14:36
Three findings from a thorough re-sweep of the codebase for residual
PR-B-vocabulary references:

1. compressionCommand() still routed `enable` / `disable` subcommands
   to a Phase-0 stub (return OK silently), and the HELP text listed
   `ENABLE, DISABLE, SWEEP, TRAIN` as future Phase-1 work. Per
   PR-B's design §4.5, the ENABLE/DISABLE aliases are NOT part of
   v1 (the 3-state enum doesn't map cleanly to enable/disable
   verbs). Removed the stub branch and rewrote the HELP text to
   reflect the v1 surface (STATUS / HELP land here; SWEEP FORCE /
   TRAIN / DICT * land in subsequent S2 PRs).

2. Three code comments cited "R2.1.5" when describing the "no
   active dictionary" state. Per the post-PR-B design that's
   actually R2.1.7 (master=compression but no active dict yet —
   the third state). Updated:
     - src/compression.c:1158 (compressionEnqueueCandidate's
       no-dict short-circuit)
     - src/compression.h:299 (header doc for the same)
     - src/compression_workers.c:619 (worker err>0 worker-policy
       sentinel comment)

3. .agents/planning/realtime-data-compression/idea-honing.md's
   PR-B header note said "Q1-Q16 hold up unchanged" but Q15
   (testing strategy — §7.1 transparency-mode harness) describes
   a `--compression` flag whose server-config payload uses the
   old `--compression-enabled yes` etc. The flag's design intent
   is still valid, but the specific config names are superseded.
   Tightened the header note to flag Q15 explicitly and revise
   the unchanged-Qs list (Q1-Q4, Q6-Q14, Q16).

Verified:

  make -j2 -C src SERVER_CFLAGS=-Werror   # clean (BUILD_ZSTD=yes)
  ./runtest --single unit/type/compression  # 13/13 pass
…uct comments

CI's clang-format-18 wants:

1. src/compression.c masterSwitchName(): switch-case 'return' values
   should NOT be column-aligned to the longest case label. The
   default LLVM 18 style collapses each 'case X: return Y;' onto a
   single line with one space.

2. src/server.h compressionState struct: the new
   compression_active_sweeper_interval field (35 chars) is longer
   than the prior longest field (compression_max_value_size at 26
   chars), so the trailing-comment alignment column for ALL fields
   in this block needs to shift right (from column 39 to column 46).
   Also the continuation lines wrapping the multi-line comments
   need to align with the new comment column.

Both findings reported by the failed clang-format-check job on
PR #29 (CI is the source of truth here — clang-format-18 isn't
installed locally). Verified the build still passes after the
formatting fix.
@ikolomi ikolomi merged commit 8748cc8 into unstable Jun 14, 2026
79 of 80 checks passed
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.

1 participant