Conversation
Seven route-taking commands documented return shapes that disagree with glide-core's response policy (ResponsePolicy::for_command in glide-core/redis-rs/redis/src/cluster_routing.rs). - bgsave, bgrewriteaof: default to all primaries with no response policy, so the cluster reply is a per-node Hash even with no route; @return said String. - lastsave, lolwut, function_dump, function_list: a multi-node route yields a per-node Hash; @return listed only the scalar. - randomkey: claimed a per-node Hash, but RANDOMKEY's FirstSucceededNonEmptyOrAllEmpty policy always collapses to a single key, or nil when every shard is empty. - function_stats: claimed a Hash keyed by "host:port" unconditionally, but a single-node route returns that node's stats unkeyed. Docs only; no behavior change. All aggregation happens in glide-core, and every affected method is a bare send_command pass-through. Cross-checked against the Go, Java, Python and Node clients, which express the same split through ClusterValue / TClusterResponse / ClusterResponse. Signed-off-by: Alex Le <alex.le@improving.com>
Signed-off-by: Alex Le <alex.le@improving.com>
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
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.
Summary
Seven route-taking commands documented cluster return shapes that disagree with what glide-core actually returns. This is a docs-only correction — no behavior change.
All cross-node aggregation happens in glide-core (
ResponsePolicy::for_commandinglide-core/redis-rs/redis/src/cluster_routing.rs); the Ruby client performs none, and every method touched here is a baresend_commandpass-through. The bug was purely that the YARD@returntags described the wrong shapes, so users reading the docs would write code against a scalar where a per-nodeHasharrives (or vice versa).Issue link
None — found while auditing Ruby's cluster response handling against the Go, Java, Python and Node clients.
Changes
Corrections in three directions:
Scalar documented, per-node
Hashpossible —@returnwidened to a union:bgsave,bgrewriteaof[String][String, Hash{String => String}]lastsave[Integer][Integer, Hash{String => Integer}]lolwut[String][String, Hash{String => String}]function_dump[String][String, Hash{String => String}]function_list[Array<Hash>][Array<Hash>, Hash{String => Array<Hash>}]BGSAVE/BGREWRITEAOFare the notable ones: they route to all primaries by default with no response policy, so on cluster they return a per-nodeHasheven with noroute:argument, not just under an explicit multi-node route.Hashdocumented, never returned —randomkeyclaimed "When routed, may return a Hash of node => value". It cannot:RANDOMKEYcarriesResponsePolicy::FirstSucceededNonEmptyOrAllEmpty, which always collapses to a single key, ornilwhen every shard is empty.Hashover-promised —function_statsclaimed aHashkeyed by"host:port"unconditionally.FUNCTION STATSisResponsePolicy::Specialwith an all-nodes default, so the no-route reply is keyed by node, but a single-node route (e.g.Valkey::Route.random) returns that node's stats unkeyed.These now match how the peer clients type the same commands: Java
ClusterValue<T>, PythonTClusterResponse[T], NodeClusterResponse<T>, GoClusterValue[T]. Commands already documented correctly (dbsize's aggregated sum,ping/flushall/flushdb/config_set/config_resetstat/config_rewrite/savecollapsing to a single"OK"/"PONG", and theecho/client_id/time/config_get/infounions) are untouched.Limitations
Docs only — no runtime behavior, no new tests. Two related gaps found during the audit are deliberately left out of scope:
test/integration/cluster/cluster_routing_test.rbasserts onlyIntegerand>= 0forDBSIZEwithall_primaries, so it verifies the collapse but never that the value is genuinely summed. Java, Python and Node all assert the exact total.flatten_map: trueflattens every MAP response including the per-node cluster map, which would turninfo(route: Route.all_primaries)into a flat Array of unparsed node strings.test/integration/valkey/flatten_map_test.rbis standalone-only, so nothing covers the combination.Testing
bundle exec rubocopon the three changed files — 3 files inspected, no offenses.bundle exec rake test:unit— 352 tests, 631 assertions, 0 failures, 0 errors, 0 skips.No integration run: the change contains no executable code. Every claim above was verified by reading
ResponsePolicy::for_command(cluster_routing.rs:583-636),base_routing(:653-813), the multi-node fold atcluster_async/mod.rs:2324-2512, and the FFI route construction atffi/src/lib.rs:4646-4664, against mono-repo revision794486c9a.Checklist
Before submitting the PR make sure the following are checked:
git commit -s) per the DCO.bundle exec rubocop) and pass.