Skip to content

Untrademark json files#35

Merged
madolson merged 6 commits into
valkey-io:unstablefrom
zuiderkwast:untrademark-json-files
Mar 26, 2024
Merged

Untrademark json files#35
madolson merged 6 commits into
valkey-io:unstablefrom
zuiderkwast:untrademark-json-files

Conversation

@zuiderkwast

@zuiderkwast zuiderkwast commented Mar 26, 2024

Copy link
Copy Markdown
Contributor

Replaces #26

Name agnostic. Now without spelling errors, ready to merge if you ask me.

@zuiderkwast zuiderkwast requested review from hwware and madolson March 26, 2024 10:24
@zuiderkwast zuiderkwast mentioned this pull request Mar 26, 2024
18 tasks
Comment thread src/commands/README.md
Comment thread src/commands/command-docs.json
Comment thread src/commands/command-docs.json
],
"reply_schema": {
"description": "the number of clients that received the message. Note that in a Redis Cluster, only clients that are connected to the same node as the publishing client are included in the count",
"description": "the number of clients that received the message. Note that in a Cluster, only clients that are connected to the same node as the publishing client are included in the count",

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(Not a blocker here) This documentation sits wrong with me, since this is like, only for cluster mode. @hpatro maybe take a look at this and fix it later?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point, but let's not get stuck on these details.

We could also use the word "cluster" meaning a whole cluster (all the nodes) and "cluster-enabled" when we're talking about the configured mode of the current node.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@madolson didn't follow you exactly, SPUBLISH also works in standalone mode.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. the behavior is the same for both cluster and non-cluster though right? It's only the name of clients connected to the current node. I guess the documentation on publish is also weird.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, we can get rid of Note that in a Cluster.

@madolson madolson left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some nitpicky stuff that isn't strictly related.

@madolson madolson merged commit 3782446 into valkey-io:unstable Mar 26, 2024
@zuiderkwast zuiderkwast deleted the untrademark-json-files branch March 26, 2024 21:41
@PingXie PingXie added the rebranding Valkey is not Redis label Mar 31, 2024
PatrickJS pushed a commit to PatrickJS/placeholderkv that referenced this pull request Apr 24, 2024
Replaces valkey-io#26

Name agnostic. Now without spelling errors, ready to merge if you ask me.
GilboaAWS pushed a commit to GilboaAWS/valkey that referenced this pull request Jun 24, 2026
* [Topic-2 PR-A] COMPRESSION DICT-IMPORT + runtime dict-generation test infra (R2.3.10)

Implements the minimal preshared-dictionary import surface so
integration tests can run before S1.x training lands. R2.3.10 + §4.5
in the design doc: operator base64-encodes a ZSTD-trained dictionary
and installs it via `COMPRESSION DICT-IMPORT <base64-bytes>`. The
new dict is promoted as active; the previous active is retired
through the existing registry path.

The blocker this solves: `compressionEnqueueCandidate` early-returns
when `compressionRegistryActive() == NULL`, so without a trained
dict the entire write/sweep path is a no-op. Tests that exercise
end-to-end compression behaviour (S2.7 write hook, S2.8 read hook,
S2.9 sweeper) need a way to install a dict; this PR is that way.
S1.x's full training implementation (BIO_COMPRESSION_TRAIN +
ZDICT_trainFromBuffer on the bio thread) lands separately on the
@GilboaAWS track.

Implementation
--------------

Hyphenated subcommand `DICT-IMPORT` (CLUSTER COUNT-FAILURE-REPORTS
precedent — RESP doesn't have nested subcommand containers).

Validation, in order:
  - Base64 decoding (private static `base64Decode` in compression.c;
    standard alphabet, optional `=` padding, whitespace rejected).
  - 4-byte ZSTD magic 0xEC30A437 — rejects raw-content prefixes and
    other non-trained bytes. Exotic operators with raw prefixes will
    have to find another route; the 99% case is "I trained a dict,
    I'm importing it" and that case wants real validation.
  - `ZSTD_createCDict` / `ZSTD_createDDict` — these accept arbitrary
    bytes as raw prefixes (never return NULL on garbage), so the
    magic check above is the actual content-validity gate. The ZSTD
    calls remain as belt-and-suspenders for OOM and similar.
  - `compressionRegistryAdd(pair, promote=1)` — same path a trained
    dict will use once S1.x lands.

Reply: integer dict_id on success, RESP error on rejection.

INFO renderer
-------------

Replaced the `compression_active_dict_id:0` and `compression_known_
dicts:0` placeholders with live values via the new registry accessor
`compressionRegistryGetKnownCount()`. Operators running this server
can now see imported dicts immediately in `INFO compression` /
`COMPRESSION STATUS`. Other field placeholders (compressed_objects,
ratio, etc.) remain at 0 until later S2 PRs land their counters.

Test infrastructure: runtime dict generation
--------------------------------------------

Static dict fixtures don't scale to the test matrix the project
needs (per-shape dicts for JSON / kv / log workloads, drift testing
where the dict was trained on shape A but workload arrives as shape
B, retraining cycles where dict A is replaced by dict B). Shipping
multiple ~10 KiB binaries under tests/assets/ would bloat the repo
and still not cover the drift case. Instead, we generate dicts at
test time, on demand, parametrized by data shape.

The dict generator MUST be external to valkey-server. If we used a
server-side test command (e.g. DEBUG COMPRESSION TRAIN-FROM-BYTES),
a bug in the server's training plumbing could mask itself — both
the test fixture and the production training path would share code
and exhibit the same bug. The infrastructure landed here uses a
separate process that calls only ZDICT_trainFromBuffer directly:

  - tests/helpers/gen-zstd-dict.c (new):
    Standalone helper binary. Reads samples from stdin in a simple
    binary protocol (4-byte big-endian length + N bytes per sample,
    repeated until EOF), trains a ZSTD dictionary via
    ZDICT_trainFromBuffer, writes the trained dict to a path passed
    on argv. Links against the same vendored deps/zstd/libzstd.a as
    valkey-server, so ZDICT API behaviour matches what production
    will use, but runs in a separate process with no shared memory
    or globals with the SUT.

  - src/Makefile:
    Adds tests/helpers/gen-zstd-dict to ALL_BUILD_PREREQUISITES when
    BUILD_ZSTD=yes (gated by the same ifeq block that controls the
    feature itself). BUILD_ZSTD=no skips it — there's no compression
    feature to test. clean target updated.

  - tests/support/compression-helpers.tcl (new):
    Sample generators (gen_kv_samples, gen_json_samples,
    gen_log_samples) producing reproducible per-seed sample lists.
    gen_drifted_samples mixes two shapes by a `drift` fraction in
    [0,1] for drift / retraining tests. train_dict_from_samples
    pipes samples through the helper binary; import_dict is the
    convenience wrapper that trains + base64-encodes + sends
    COMPRESSION DICT-IMPORT.

  - tests/test_helper.tcl: source the new support file.

  - tests/unit/type/compression.tcl: the existing "import a real
    trained dict" Tcl test now generates samples + trains at test
    time instead of reading a static fixture. New "drift mixer
    sanity" test verifies the gen_drifted_samples helper itself.

  - tests/assets/test-compression.dict: deleted (no longer needed).

Tests
-----

gtest (392 total, +1 new under CompressionRegistryTest):
  - GetKnownCountTracksAddsAndCapEnforcement — verifies the new
    accessor moves with each Add and stays at the cap on rejection.

Tcl (28 total, +6 new under unit/type/compression):
  - Rejects malformed base64.
  - Rejects valid base64 without ZSTD magic ("hello world").
  - Rejects payloads smaller than the magic header.
  - Validates arity at the command-table level.
  - Imports a real runtime-trained dict, verifies INFO reflects
    active_dict_id+known_dicts, second import promotes new + retires
    previous (count=2).
  - Smoke-tests gen_drifted_samples (verifies pure-A / pure-B / 50-50
    mixing produce shape-distinct outputs).

Verification
------------

  - 392 gtests pass (was 391 — +1 new).
  - 28 Tcl tests pass (was 22 — +6 new).
  - BUILD_ZSTD=yes and BUILD_ZSTD=no both clean with -Werror.
  - gen-zstd-dict helper builds only when BUILD_ZSTD=yes and is
    invoked correctly by the Tcl wrapper end-to-end.

Out of scope for this PR
------------------------

  - DICT-EXPORT (R2.3.10 mentions both; symmetric implementation is
    a small follow-up once we have one operator who needs it).
  - DICT-LIST / DICT-DROP (§4.5; pending S4.x observability work).
  - Real training (S1.x — @GilboaAWS track).
  - Topic-2 PR-B (compression-stress.tcl) — the integration stress
    test that USES this command. Lands next.

* [Topic-2 PR-B] compression-stress.tcl integration tests + strEncoding fix (valkey-io#34)

* [Topic-2 PR-A] COMPRESSION DICT-IMPORT + runtime dict-generation test infra (R2.3.10) (valkey-io#33)

Implements the minimal preshared-dictionary import surface so
integration tests can run before S1.x training lands. R2.3.10 + §4.5
in the design doc: operator base64-encodes a ZSTD-trained dictionary
and installs it via `COMPRESSION DICT-IMPORT <base64-bytes>`. The
new dict is promoted as active; the previous active is retired
through the existing registry path.

The blocker this solves: `compressionEnqueueCandidate` early-returns
when `compressionRegistryActive() == NULL`, so without a trained
dict the entire write/sweep path is a no-op. Tests that exercise
end-to-end compression behaviour (S2.7 write hook, S2.8 read hook,
S2.9 sweeper) need a way to install a dict; this PR is that way.
S1.x's full training implementation (BIO_COMPRESSION_TRAIN +
ZDICT_trainFromBuffer on the bio thread) lands separately on the
@GilboaAWS track.

Implementation
--------------

Hyphenated subcommand `DICT-IMPORT` (CLUSTER COUNT-FAILURE-REPORTS
precedent — RESP doesn't have nested subcommand containers).

Validation, in order:
  - Base64 decoding (private static `base64Decode` in compression.c;
    standard alphabet, optional `=` padding, whitespace rejected).
  - 4-byte ZSTD magic 0xEC30A437 — rejects raw-content prefixes and
    other non-trained bytes. Exotic operators with raw prefixes will
    have to find another route; the 99% case is "I trained a dict,
    I'm importing it" and that case wants real validation.
  - `ZSTD_createCDict` / `ZSTD_createDDict` — these accept arbitrary
    bytes as raw prefixes (never return NULL on garbage), so the
    magic check above is the actual content-validity gate. The ZSTD
    calls remain as belt-and-suspenders for OOM and similar.
  - `compressionRegistryAdd(pair, promote=1)` — same path a trained
    dict will use once S1.x lands.

Reply: integer dict_id on success, RESP error on rejection.

INFO renderer
-------------

Replaced the `compression_active_dict_id:0` and `compression_known_
dicts:0` placeholders with live values via the new registry accessor
`compressionRegistryGetKnownCount()`. Operators running this server
can now see imported dicts immediately in `INFO compression` /
`COMPRESSION STATUS`. Other field placeholders (compressed_objects,
ratio, etc.) remain at 0 until later S2 PRs land their counters.

Test infrastructure: runtime dict generation
--------------------------------------------

Static dict fixtures don't scale to the test matrix the project
needs (per-shape dicts for JSON / kv / log workloads, drift testing
where the dict was trained on shape A but workload arrives as shape
B, retraining cycles where dict A is replaced by dict B). Shipping
multiple ~10 KiB binaries under tests/assets/ would bloat the repo
and still not cover the drift case. Instead, we generate dicts at
test time, on demand, parametrized by data shape.

The dict generator MUST be external to valkey-server. If we used a
server-side test command (e.g. DEBUG COMPRESSION TRAIN-FROM-BYTES),
a bug in the server's training plumbing could mask itself — both
the test fixture and the production training path would share code
and exhibit the same bug. The infrastructure landed here uses a
separate process that calls only ZDICT_trainFromBuffer directly:

  - tests/helpers/gen-zstd-dict.c (new):
    Standalone helper binary. Reads samples from stdin in a simple
    binary protocol (4-byte big-endian length + N bytes per sample,
    repeated until EOF), trains a ZSTD dictionary via
    ZDICT_trainFromBuffer, writes the trained dict to a path passed
    on argv. Links against the same vendored deps/zstd/libzstd.a as
    valkey-server, so ZDICT API behaviour matches what production
    will use, but runs in a separate process with no shared memory
    or globals with the SUT.

  - src/Makefile:
    Adds tests/helpers/gen-zstd-dict to ALL_BUILD_PREREQUISITES when
    BUILD_ZSTD=yes (gated by the same ifeq block that controls the
    feature itself). BUILD_ZSTD=no skips it — there's no compression
    feature to test. clean target updated.

  - tests/support/compression-helpers.tcl (new):
    Sample generators (gen_kv_samples, gen_json_samples,
    gen_log_samples) producing reproducible per-seed sample lists.
    gen_drifted_samples mixes two shapes by a `drift` fraction in
    [0,1] for drift / retraining tests. train_dict_from_samples
    pipes samples through the helper binary; import_dict is the
    convenience wrapper that trains + base64-encodes + sends
    COMPRESSION DICT-IMPORT.

  - tests/test_helper.tcl: source the new support file.

  - tests/unit/type/compression.tcl: the existing "import a real
    trained dict" Tcl test now generates samples + trains at test
    time instead of reading a static fixture. New "drift mixer
    sanity" test verifies the gen_drifted_samples helper itself.

  - tests/assets/test-compression.dict: deleted (no longer needed).

Tests
-----

gtest (392 total, +1 new under CompressionRegistryTest):
  - GetKnownCountTracksAddsAndCapEnforcement — verifies the new
    accessor moves with each Add and stays at the cap on rejection.

Tcl (28 total, +6 new under unit/type/compression):
  - Rejects malformed base64.
  - Rejects valid base64 without ZSTD magic ("hello world").
  - Rejects payloads smaller than the magic header.
  - Validates arity at the command-table level.
  - Imports a real runtime-trained dict, verifies INFO reflects
    active_dict_id+known_dicts, second import promotes new + retires
    previous (count=2).
  - Smoke-tests gen_drifted_samples (verifies pure-A / pure-B / 50-50
    mixing produce shape-distinct outputs).

Verification
------------

  - 392 gtests pass (was 391 — +1 new).
  - 28 Tcl tests pass (was 22 — +6 new).
  - BUILD_ZSTD=yes and BUILD_ZSTD=no both clean with -Werror.
  - gen-zstd-dict helper builds only when BUILD_ZSTD=yes and is
    invoked correctly by the Tcl wrapper end-to-end.

Out of scope for this PR
------------------------

  - DICT-EXPORT (R2.3.10 mentions both; symmetric implementation is
    a small follow-up once we have one operator who needs it).
  - DICT-LIST / DICT-DROP (§4.5; pending S4.x observability work).
  - Real training (S1.x — @GilboaAWS track).
  - Topic-2 PR-B (compression-stress.tcl) — the integration stress
    test that USES this command. Lands next.

* [Topic-2 PR-B] compression-stress.tcl integration tests + 2 prerequisite fixes

Adds tests/integration/compression.tcl — first end-to-end exercise of
the merged S2.x stack against a real workload. Builds on top of PR-A
(valkey-io#33)'s runtime dict-generation infrastructure: each test imports a
freshly-trained dict via tests/support/compression-helpers.tcl, then
exercises one specific behaviour of the hot path.

Six test cases:

  1. Write-path round trip.
     master=compression + sweeper=disabled. SET a compressible value,
     poll until OBJECT ENCODING reports "compressed", verify GET
     round-trips the original bytes through the read-path transient
     view (R2.5.7).

  2. Sweeper compresses pre-existing keys.
     master=off + populate 100 RAW keys, then flip to
     master=compression + sweeper=enabled. Wait for ALL keys
     compressed; verify EVERY value round-trips (no spot-checks).

  3. Decompression drain.
     Continuing from valkey-io#2's compressed state, flip to
     master=decompression + sweeper=enabled. Wait for ALL keys
     drained back to RAW; verify EVERY value round-trips.

  4. COMPRESSION SWEEP FORCE end-to-end.
     master=compression + sweeper=disabled (manual-only). Populate
     uncompressed, then run COMPRESSION SWEEP FORCE. Wait for ALL
     keys to be compressed by a single forced pass; verify EVERY
     value round-trips.

  5. Mixed workload preserves data integrity under live sweeper.
     master=compression + sweeper=enabled + 50% pacing. Populate 200
     keys, run 500 random ops (GET/SET/APPEND/SETRANGE/DEL).

  6. Ineligibility — values outside the size envelope and hot keys.
     Verifies the eligibility predicate (R2.2): values below
     compression-min-value-size, values above compression-max-value-size,
     and freshly-written hot keys must NOT be compressed even with the
     sweeper running at maximum cadence.

Prerequisite fix 1: src/object.c strEncoding()
================================================

OBJECT ENCODING was returning "unknown" for compressed values
because strEncoding() didn't have a case for OBJ_ENCODING_COMPRESSED.
Design R2.7.1 requires it returns "compressed". One-line fix that
slipped through earlier S2 PRs.

Prerequisite fix 2: src/compression.c compressionEnqueueCandidate()
====================================================================

Use-after-free caught by AddressSanitizer on the first PR-B CI run.
The eligibility predicate accepts encoding==RAW; a value currently
in transient-view state (R2.5.7) reads as RAW because val_ptr is
the per-iteration temp uncompressed sds. compressionEnqueueCandidate
would then capture job->src = temp_sds, which restoreTransientEntry
frees at beforeSleep — leaving the worker thread's job->src dangling
into freed memory.

Fix: gate the enqueue on !transientViewActive(value). Skipping the
enqueue is functionally harmless — a value in transient view is
already compressed (the original frame is saved in the side-map and
will be restored at beforeSleep). One line added at the top of
compressionEnqueueCandidate, with an explanatory comment naming the
exact ASan trace it fixes.

Tests: 392 gtests + 34 Tcl tests pass (28 unit + 6 integration).
Both BUILD_ZSTD={yes,no} build clean with -Werror. Verified the
asan fix locally by rebuilding with -fsanitize=address and re-running
the integration suite — no use-after-free.
GilboaAWS pushed a commit to GilboaAWS/valkey that referenced this pull request Jun 24, 2026
….7 stickiness paragraph (valkey-io#36)

R2.8.4 — savings are workload-dependent; existing observability is
sufficient. Documents three interactions Topic-2 PR-B (the integration
stress test) made it natural to revisit:

- The same maxmemory setting yields different effective capacity
  depending on whether the workload is compressible (capacity grows by
  1/ratio) or not (unchanged minus ~16 B/key header + fixed overhead).
- Eviction interaction: per R2.8.1 the sampler sees compressed
  footprints, so eviction triggers at maxmemory measured in compressed
  bytes; victim selection by LRU/LFU is unchanged (no bias from
  per-key ratio asymmetry); evicted_keys is the canonical signal.
- Decompress-mode drain causes used_memory to grow toward the
  no-compression baseline as the sweeper + permanent-decompress
  reclaim compressed frames; operators with active maxmemory should
  ensure headroom before flipping.

Decision-record: no new compression-specific metric introduced for v1.
PR-B's stress test surfaced no observation that would justify one.
The existing fields plus the planned compression_transient_view_capped_total
(S4.1) cover the operator-facing diagnostic surface. Revisit after
S5.x benchmark scenarios.

R2.5.7 — added stickiness paragraph cross-referenced from R2.5.6.
Spells out that a value permanently decompressed (drain-mode or
write-path mutation) stays RAW until either the sweeper visits it
under master=compression (gated by compression-min-idle-seconds, by
design to avoid worker-pool churn) or until eviction/expire/DEL
makes the question moot. No immediate auto-recompress path; the
sweeper pacing bounds the re-compress overhead globally.

plan.md updated:
- Topic-2 PR-B marked complete (merged at unstable@7013e370b via valkey-io#35).
- Topic-1 marked complete (lands via this same PR).

Docs only — no code change. Build is unchanged.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

rebranding Valkey is not Redis

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants