Request response policies#2996
Draft
nkaradzhov wants to merge 26 commits into
Draft
Conversation
nkaradzhov
force-pushed
the
request-response-policies
branch
2 times, most recently
from
e74bb9a to
2ea355f
Compare
nkaradzhov
force-pushed
the
request-response-policies
branch
from
fd526fa to
e857c1b
Compare
for now we only extract: - request and response policy -> from tips - is the command keyless -> from key specs
actually fetched from server using the dynamic resolver
Move each switch case body in `_execute` into a typed router/reducer function in `request-response-policies/dispatch.ts`. Switches still dispatch by policy enum; behavior unchanged. Prepares for replacing the switches with a strategy registry in the next commit. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Define `REQUEST_ROUTERS` and `RESPONSE_REDUCERS` as policy-keyed records of the dispatch helpers introduced in the previous commit. The two switches in `_execute` collapse to a single registry lookup each, with `Unknown policy` errors preserved. `satisfies Record<PolicyName, ...>` keeps the lookup exhaustive at the type level. Adding a new policy now requires only a new entry in the registry — no `_execute` changes. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- `CommandIdentifier.subcommand` is now `string | undefined`, mirroring the fact that single-word commands have no second arg. The parser emits `undefined` instead of an implicit empty value. - `StaticPolicyResolver` lowercases the entire policy table at construction time, so lookups are case-insensitive regardless of the casing used in the static data file (which today mixes upper- and lowercase module/command keys). - Subcommand lookups also lowercase the incoming identifier and guard against `undefined`, so `MEMORY USAGE` resolves to the `usage` subcommand policy regardless of caller casing. - Drop the stray `console.log` left in the resolver. - Error messages that referenced `parser.commandIdentifier` now print a `command [subcommand]` label instead of `[object Object]`. - Update `dynamic-policy-resolver.spec.ts` to pass `CommandIdentifier` objects to `resolvePolicy` (previously was passing raw strings). - Add `static-policy-resolver.spec.ts` covering FT.SEARCH, MEMORY USAGE, GET, COMMAND INFO, FT.SUGADD, casing, errors and fallback. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The static policy table had three problems for the Search module: - The `FT` module entries were upper-cased while every other module uses lower-case keys. - A second `search` module held three RediSearch cluster-admin commands (`CLUSTERSET`, `CLUSTERINFO`, `CLUSTERREFRESH`) that are not part of the public FT.* surface. - A `_FT` module held private debug subcommands (`_FT.CONFIG`, `_FT.DEBUG`, etc.) and a long list of deprecated / private FT.* entries (`_LIST`, `_CREATEIFNX`, `_DROPIFX`, `_DROPINDEXIFX`, `_ALIASADDIFNX`, `_ALIASDELIFX`, `_ALTERIFNX`, `ADD`, `DEL`, `GET`, `MGET`, `SYNADD`) which are not user-facing and shouldn't participate in policy resolution. Replace those three modules with a single lower-case `ft` module containing exactly the 25 commands listed in the HLD Command Routing Policy Table, each with the policy the client should apply per the HLD client-interpretation column. `ft.cursor` is left at `default-keyless`/`default-keyless` for now; the HLD specifies `special` (sticky cursor) but that depends on the not-yet-built special-handler registry and cursor binding map. Add `ft-policies.spec.ts`, a parameterised test that asserts every HLD command resolves to the prescribed `request`/`response` pair and that the dropped debug / admin commands no longer resolve. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- extract DynamicPolicyResolverFactory.buildModulePolicyRecords so the generator and the runtime resolver share one derivation path - scripts/generate-static-policies-data.ts (npm run generate:policies -- <redis-url>) regenerates static-policies-data.ts from a live COMMAND reply; keys lowercased and sorted for stable diffs - HLD curation codified in scripts/static-policies-overrides.ts: internal/deprecated FT commands and _ft/search cluster-admin modules excluded, ft.cursor pinned to default-keyless until the special-handler registry lands - data regenerated against Redis 8.8.0: adds 42 new std commands (msetex was missing entirely), json.debug flips to keyless per server-reported key specs Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
COMMAND tips carry no ordering guarantee; commands like INFO declare nondeterministic_output before their policy tips, so positional parsing silently dropped request/response policies for 13 commands (scan, info, memory/latency subcommands, function stats, cluster slot-stats, hotkeys get) and they fell back to default routing. Scan the whole tips array by prefix instead and regenerate static-policies-data from Redis 8.8.0. Surfaced 'special' policies now reach the throwing routeSpecial / reduceSpecial handlers on cluster dispatch; safe fallback deferred.
Key specs were reduced to an isKeyless boolean; the begin_search /
find_keys structures were discarded. The multi_shard splitter needs them
as the reconstruction recipe (key strides, keynum count position,
options suffix), so retain them as CommandReply.keySpecs.
RESP2 delivers each map level as flat field-value pair arrays; normalize
to the RESP3 object shape so both protocols parse identically.
Unrecognized or malformed entries parse to { type: 'unknown' } instead
of throwing — consumers decide whether unknown is acceptable.
The multi_shard splitter needs the key-spec reconstruction recipe at resolution time on both resolver paths. Copy CommandReply.keySpecs into CommandPolicies only for multi_shard commands — the same builder produces static-policies-data.ts, and unconditional copying would roughly triple the generated file with specs nothing reads. Regenerated static data (Redis 8.8.0): del, exists, mget, mset, msetex, touch, unlink gained keySpecs; msetex is the only keynum spec.
Pure per-slot splitter driven by COMMAND key specs: range specs cover DEL, UNLINK, EXISTS, TOUCH, MGET, MSET; keynum covers MSETEX (numkeys rewritten per sub-command, options suffix copied verbatim). Single-slot commands pass through unsplit, preserving single-slot atomicity. Each sub-command records its key groups' original ordinals so reply aggregation can restore input key order (MGET). Anything that cannot be split deterministically — missing/multiple/keyword/unknown specs, malformed numkeys, misaligned key region — throws naming the command: a wrong split of a write command means corrupted data, so refusal beats guessing. Routing/aggregation wiring lands separately.
Restore _execute to a policy-free transport primitive — one command to one client (key-routed unless a pinned client is given) with MOVED/ASK redirect handling. Policy resolution, request-policy fan-out and response-policy aggregation move to _executeWithPolicies, which call sites hand data (parser, command, transformReply) instead of fn closures; fn creation now happens in one place, which the multi_shard splitter wiring will rely on to build per-sub-command closures. No behavior change: cluster test results are identical before and after. Note: _executePolicyPlan is a _-prefixed method rather than a native #private — cluster instances are derived via Object.create, which does not carry private fields.
reduceDefaultKeyed returned the responses array instead of the single reply, wrapping every keyed cluster command (GET returned ['value']). reduceDefaultKeyless fed single replies through merge aggregation, which throws on scalars (PUBLISH replies are numbers); merging is only meaningful for fan-out replies, so single-target responses now pass through. Also drop a stray debug console.log from aggregateMerge.
getAllMasterClients/getAllClients silently skipped nodes without a connected client. With minimizeConnections (or a partially connected cluster) all_shards/all_nodes commands fanned out to a subset of nodes — or none, resolving undefined: PING via sendCommand returned undefined, DBSIZE could sum a subset without any error. Route through nodeClient(), the same lazy connect-or-reuse accessor keyed routing uses. getAllClients no longer yields dedicated PubSub connections; they cannot run regular commands. Also guard the policy dispatch against an empty fan-out — failing loud beats resolving undefined.
Make the multi_shard request policy actually split commands per hash slot instead of sending the full command to one client per key. - Routers now return a plan (RoutedCommand[]) rather than bare clients: pass-through policies set `client`; routeMultiShard calls the splitter and builds a sub-parser per slot (buildSubParser marks keys at the splitter's new keyPositions so `firstKey` routes each sub-command). - The engine runs each plan entry through core `_execute` with its own sub-parser, so every shard gets its own sub-args; MOVED/ASK use the sub-command's key. - Response reducers gain optional positionHints; reduceDefaultKeyed scatters split replies by groupIndices so MGET preserves caller key order across slots. Aggregating reducers (agg_sum, all_succeeded) ignore the hint. - Splitter SubCommand now carries keyPositions for sub-parser key marking. Also merge _executePolicyPlan into _executeWithPolicies — the old name implied a plan object that never existed; it is now the single engine (resolve policy -> plan -> execute -> reduce). Router/reducer types are intentionally non-generic (routing sits below the typed command surface; clients are opaque pass-through), erased to the base cluster types instead of `any`; the engine re-narrows the client at the `_execute` boundary. Working end-to-end: DEL/UNLINK/EXISTS/TOUCH, MSET/MSETEX, MGET. Raw sendCommand multi_shard and cluster-docker integration tests remain. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
_executeWithPolicies routed every command through policy resolution, but
the cluster's resolver (StaticPolicyResolver(POLICIES)) has no fallback,
so any command absent from the policy table resolved to { ok: false }
and the engine threw "Policy resolution error". This broke user-defined
custom commands, scripts/functions whose dispatched name is not in the
table, and module commands the resolver was not built with.
Such commands have no request/response policy and nothing to split or
aggregate. Instead of throwing, fall back to a default policy
(default_keyed when the parser carries keys, else default_keyless) and
route through the existing pass-through default router/reducer — the
same single-client, key-routed behaviour as a direct _execute. Known
module commands the dynamic resolver does recognise still get their real
policy, so a module declaring multi_shard keeps working. Known commands
that cannot be split still throw from the splitter.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The policy refactor folded the raw cluster sendCommand into _executeWithPolicies but built its parser wrong, regressing behaviour master got right by routing on the explicit firstKey: - firstKey was prepended to args, so redisArgs[0] became a key instead of the command name (breaking policy resolution) and no key was ever marked, leaving parser.firstKey unset so routing fell back to undefined. - makeFn ignored the per-entry sub-parser and always sent the original args, so a split multi_shard command sent the full command to every shard. Build redisArgs as an exact copy of args (command name at index 0) and register the caller's firstKey via a new BasicCommandParser.markRoutingKey, which records the key for routing without re-appending it to redisArgs. Send p.redisArgs from the closure so split sub-commands each carry their own slot's arguments. A known multi_shard command sent raw now splits correctly, since the resolver supplies its key specs and the splitter operates on the flat redisArgs. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
nkaradzhov
force-pushed
the
request-response-policies
branch
2 times, most recently
from
624384f to
a85904d
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Checklist
npm testpass with this change (including linting)?