Consolidate redundant command implementations into BaseClient - #498
Open
currantw wants to merge 3 commits into
Open
Consolidate redundant command implementations into BaseClient#498currantw wants to merge 3 commits into
currantw wants to merge 3 commits into
Conversation
…BaseClient (valkey-io#462) - Move 19 connection/server management commands from abstract+override pattern to concrete implementations in BaseClient - Fix incorrect cluster routing: - SELECT: Random → AllNodes (valkey-io#491) - CONFIG SET: AllPrimaries → AllNodes (valkey-io#492) - CONFIG REWRITE: Random → AllNodes (valkey-io#493) - CONFIG RESETSTAT: AllPrimaries → AllNodes (valkey-io#493) - FUNCTION KILL: AllPrimaries → AllNodes (valkey-io#494) - Remove isSingleValue parameter from ToClusterValue; inline MakeClusterValueHandler into Cmd.ToClusterValue() - Update GetServerVersionAsync to use default routing - Mark deprecated methods for removal in 2.0 (valkey-io#495, valkey-io#496): - ConfigGetAsync(ValkeyValue): use routed overload instead - FunctionListAsync(FunctionListOptions?): use routed overload instead - ScriptInvokeAsync(Script, ClusterScriptOptions): wrong design - ClusterScriptOptions: remove in favor of ScriptOptions - Delete GlideClient.ConnectionManagementCommands.cs Signed-off-by: Taylor Curran <taylorcurran@outlook.com> Signed-off-by: currantw <taylor.curran@improving.com>
currantw
force-pushed
the
462-redundant-implementations
branch
from
July 30, 2026 22:00
6254d53 to
1a24e7f
Compare
There was a problem hiding this comment.
Pull request overview
This PR refactors the C# client command surface by moving previously duplicated connection/server management command implementations into BaseClient, while also correcting several cluster-routing defaults by relying on glide-core’s built-in routing.
Changes:
- Consolidated previously
abstract+ duplicated overrides into concreteBaseClientimplementations (reducing duplication across standalone/cluster clients). - Fixed cluster-mode routing for several commands by removing incorrect explicit
Routeusage and relying on glide-core defaults. - Simplified cluster response handling by making
Cmd.ToClusterValue()infer single-vs-multi from the response shape.
Reviewed changes
Copilot reviewed 20 out of 20 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/Valkey.Glide.UnitTests/PubSubThreadSafetyTests.cs | Removes now-unneeded mock overrides after BaseClient consolidation. |
| tests/Valkey.Glide.UnitTests/PubSubFFIIntegrationTests.cs | Removes now-unneeded mock overrides after BaseClient consolidation. |
| tests/Valkey.Glide.UnitTests/ClusterScriptOptionsTests.cs | Suppresses obsolete warnings for ClusterScriptOptions tests pending removal. |
| tests/Valkey.Glide.IntegrationTests/ServerManagementCommandTests.cs | Adds targeted CS0618 suppression for deprecated cluster APIs. |
| tests/Valkey.Glide.IntegrationTests/ClusterClientTests.cs | Adds targeted CS0618 suppression for deprecated cluster APIs. |
| sources/Valkey.Glide/scripting/ClusterScriptOptions.cs | Marks ClusterScriptOptions as obsolete with removal guidance. |
| sources/Valkey.Glide/Internals/ResponseConverters.cs | Removes now-inlined MakeClusterValueHandler. |
| sources/Valkey.Glide/Internals/Request.ConnectionManagement.cs | Removes route-specific ClientGetName builder; uses ToClusterValue() at call sites. |
| sources/Valkey.Glide/Internals/Cmd.cs | Reworks ToClusterValue() to detect multi-node responses by runtime type. |
| sources/Valkey.Glide/Client/IGlideClusterClient.ServerManagementCommands.cs | Deprecates legacy no-route ConfigGetAsync overload. |
| sources/Valkey.Glide/Client/IGlideClusterClient.ScriptingAndFunctionCommands.cs | Deprecates legacy script/function APIs pending redesign. |
| sources/Valkey.Glide/Client/GlideClusterClient.ServerManagementCommands.cs | Removes redundant routing/override patterns; relies on glide-core defaults. |
| sources/Valkey.Glide/Client/GlideClusterClient.ScriptingAndFunctionCommands.cs | Updates cluster scripting/function commands to new ToClusterValue() usage. |
| sources/Valkey.Glide/Client/GlideClusterClient.cs | Updates server-version retrieval to use default routing and handle multi-node responses. |
| sources/Valkey.Glide/Client/GlideClusterClient.ConnectionManagementCommands.cs | Removes duplicated no-route overrides now provided by BaseClient. |
| sources/Valkey.Glide/Client/GlideClient.ServerManagementCommands.cs | Removes redundant overrides now implemented in BaseClient. |
| sources/Valkey.Glide/Client/GlideClient.ConnectionManagementCommands.cs | Deletes duplicated command implementation file (moved to BaseClient). |
| sources/Valkey.Glide/Client/BaseClient.ServerManagementCommands.cs | Implements consolidated server-management commands in the base class. |
| sources/Valkey.Glide/Client/BaseClient.ConnectionManagementCommands.cs | Implements consolidated connection-management commands in the base class. |
| CHANGELOG.md | Documents routing fixes for cluster mode. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Signed-off-by: currantw <taylor.curran@improving.com>
…responses When a SingleNodeRoute is used, glide-core returns a raw value (not wrapped in a multi-node dictionary). Commands whose raw response type is itself a dictionary (e.g. MemoryStats) were incorrectly detected as multi-node responses. ToClusterValue(Route) skips dictionary detection for single-node routes. Signed-off-by: Taylor Curran <taylorcurran@outlook.com> Signed-off-by: currantw <taylor.curran@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
Consolidates redundant command implementations into
BaseClientand fixes incorrect default routing in cluster mode. Commands that were declaredabstractonBaseClientand overridden identically in bothGlideClientandGlideClusterClientare now concrete methods inBaseClient, eliminating duplication.Issue Link
Closes #462.
Closes #491.
Closes #492.
Closes #493.
Closes #494.
Features and Behaviour Changes
Routing bug fixes (behavioral changes):
SELECT: was routing to Random, now uses glide-core default AllNodesCONFIG SET(both overloads): was routing to AllPrimaries, now uses AllNodesCONFIG REWRITE: was routing to Random, now uses AllNodesCONFIG RESETSTAT: was routing to AllPrimaries, now uses AllNodesFUNCTION KILL: was routing to AllPrimaries, now uses AllNodesDeprecations (non-breaking, warnings only — breaking removal scheduled for 2.0):
ConfigGetAsync(ValkeyValue)— useConfigGetAsync(ValkeyValue, Route)instead (refactor(client): align ConfigGetAsync and FunctionListAsync with other GLIDE clients #495)FunctionListAsync(FunctionListOptions?)— useFunctionListAsync(FunctionListOptions?, Route)instead (refactor(client): align ConfigGetAsync and FunctionListAsync with other GLIDE clients #495)ScriptInvokeAsync(Script, ClusterScriptOptions)— wrong design, will be replaced (fix(client): ScriptInvokeAsync defaults to AllPrimaries instead of key-based routing #496)ClusterScriptOptionsclass — will be removed in favor ofScriptOptions(fix(client): ScriptInvokeAsync defaults to AllPrimaries instead of key-based routing #496)Implementation
BaseClientGlideClient.ConnectionManagementCommands.csdeleted (all methods now in BaseClient)overridekeywords fromGlideClient.ServerManagementCommands.csandGlideClusterClient.ServerManagementCommands.csToClusterValue()— removedisSingleValueparameter; handler now dynamically detects single vs multi-node responsesMakeClusterValueHandlerintoCmd.ToClusterValue()and removed fromResponseConvertersGetServerVersionAsyncin cluster client now uses default routing (no explicit route)Limitations
ConfigGetAsync(ValkeyValue)andFunctionListAsyncare deprecated but not yet removed (breaking change deferred to 2.0, see refactor(client): align ConfigGetAsync and FunctionListAsync with other GLIDE clients #495)ScriptInvokeAsyncdesign issue deferred to 2.0 (fix(client): ScriptInvokeAsync defaults to AllPrimaries instead of key-based routing #496)Testing
No updates to testing. Existing test are sufficient.
Related Issues
Checklist
CHANGELOG.md,README.md,DEVELOPER.md, and other documentation files are updated.mainor releaseCreate merge commit if merging release branch intomain, squash otherwise.