chore(cache): deprecate ShardKeyEnabled, which changes which keys co-locate and nothing else - #178
chore(cache): deprecate ShardKeyEnabled, which changes which keys co-locate and nothing else#178cosmin-staicu wants to merge 1 commit into
Conversation
d29ad4a to
2a68775
Compare
There was a problem hiding this comment.
🟡 Changes recommended
Sample guidance remains contradictory, and one changelog guarantee depends on unmerged PR #177.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Deprecates ShardKeyEnabled while preserving existing key behavior and correcting Redis Cluster guidance.
Changes:
- Marks
ShardKeyEnabledobsolete while retaining factory behavior. - Updates documentation and changelog with migration guidance.
- Clarifies caller-defined hash tags for batching.
File summaries
| File | Description |
|---|---|
src/UiPath.Caching/Redis/DefaultRedisKeyStrategyFactory.cs |
Suppresses the intentional obsolete-property access. |
src/UiPath.Caching.Abstractions/CacheOptions.cs |
Marks ShardKeyEnabled obsolete. |
docs/reference/settings.md |
Documents deprecation and alternatives. |
docs/how-to/telemetry-and-strategies.md |
Corrects strategy guidance. |
CHANGELOG.md |
Records the deprecation. |
Review details
- Files reviewed: 5/5 changed files
- Comments generated: 2
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
2a68775 to
5b7c9bd
Compare
There was a problem hiding this comment.
🔵 Needs a closer look
Documentation remains inconsistent, and it claims behavior supplied only by the still-unmerged PR #177.
Review details
Suppressed comments (2)
docs/reference/settings.md:23
- This reference explicitly says it mirrors
samples/UiPath.Caching.Sample/appsettings.all.json, but that file still documentsShardKeyEnabledas “enable for Redis Cluster with multiple shards” (line 12). Readers following the linked binding reference will therefore still receive the recommendation this deprecation is meant to remove. Update the mirrored JSON comment (and the sample's cluster-mode guidance if it is no longer intentional) in this PR.
| `ShardKeyEnabled` | `bool` | `false` | App-wide | **Deprecated.** Wraps the cache key in a `{...}` hash tag so only the key, not `AppShortName` or the keyspace, picks the slot. That changes *which* keys share a slot, not how evenly they spread: the tag is the whole key, so it is unique per key and distributes exactly as the untagged key would, and it cannot make a multi-key batch single-slot. A caller that placed its own `{tag}` keeps the slot it chose with or without it. Its only effect is co-locating the same key across the `s` and `h` keyspaces and across apps with a different `AppShortName`. Flipping it rewrites every key, so leave it as your deployment has it. To batch across keys on a cluster, give them a shared hash tag yourself. |
CHANGELOG.md:382
- This guarantee is not present on the current base:
RedisStreamHealthMaintainerstill concatenates a separately rendered quarantine prefix and then issues a two-key delete, so those library-owned keys can remain cross-slot. GitHub PR #177 contains that fix but is still open and unmerged. Make #177 a merge prerequisite/rebase this branch onto it, or avoid publishing this behavior as already unconditional.
of which anyone asked for. Everything that matters on a cluster is now unconditional: a caller's own tag survives,
a cross-slot batch is refused with a message naming the two keys, and keys the library itself must touch together
co-locate. The property is still read, because a deployment that set it would relocate every entry if it stopped
- Files reviewed: 7/7 changed files
- Comments generated: 0 new
- Review effort level: Balanced
There was a problem hiding this comment.
🟡 Changes recommended
Several statements assume #177’s malformed-brace handling and incorrectly claim that every key relocates when the setting changes.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 7/7 changed files
- Comments generated: 7
- Review effort level: Balanced
b045f5e to
22fe5a1
Compare
There was a problem hiding this comment.
🟡 Changes recommended
The documentation incorrectly limits keyspace co-location to s and h, omitting set-cache and custom differentiators.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 7/7 changed files
- Comments generated: 2
- Review effort level: Balanced
22fe5a1 to
4f5ceb3
Compare
There was a problem hiding this comment.
🟡 Changes recommended
The suppression justification still incorrectly claims that every key would relocate.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 7/7 changed files
- Comments generated: 1
- Review effort level: Balanced
4f5ceb3 to
c9ec94c
Compare
There was a problem hiding this comment.
🟡 Changes recommended
The new guidance remains inaccurate for malformed braces, brace-containing prefixes, and reserved keyspaces not rendered by the default factory.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (1)
CHANGELOG.md:382
ReserveRedisKeyspaceonly registers a name for collision validation; it does not make the default factory render that keyspace. The built-instandpskeyspaces are counterexamples: both are reserved (ServiceCollectionExtensions.cs:66-67) but rendered byBroadcast.Redis.PrefixStrategy, which ignoresShardKeyEnabled. Restrict this list to differentiators actually passed toDefaultRedisKeyStrategyFactory.
lands on one slot across every keyspace this factory renders — `s` and `h`, the set cache's `se`, the distributed
cache's `dh`, any a package reserves, any a caller passes — and across apps with a different `AppShortName`.
- Files reviewed: 7/7 changed files
- Comments generated: 6
- Review effort level: Balanced
c9ec94c to
f96805c
Compare
There was a problem hiding this comment.
🟡 Changes recommended
Several deprecation notices omit that braces in the differentiator can override the cache key’s Redis hash tag.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 7/7 changed files
- Comments generated: 4
- Review effort level: Balanced
|
Rebased onto The rebase was clean, but it invalidated six statements in this PR. Every place warning that a key whose braces form no valid tag is "wrapped into a truncated tag" described the old behaviour: #177 made Nothing else changed — the brace-free scoping, the |
b82dea2 to
7a06383
Compare
There was a problem hiding this comment.
🟡 Changes recommended
The changelog gives contradictory outcomes for malformed-brace keys.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 8/8 changed files
- Comments generated: 1
- Review effort level: Balanced
1df418a to
866735c
Compare
There was a problem hiding this comment.
🟡 Changes recommended
Several descriptions incorrectly state that every brace in a rendered prefix supplies an earlier Redis hash tag.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 8/8 changed files
- Comments generated: 4
- Review effort level: Balanced
866735c to
60f9996
Compare
|
… than leaving it off
The flag wraps a cache key in a '{...}' hash tag so the slot follows the key
rather than AppShortName and the differentiator. For a brace-free key that
changes which keys share a slot and nothing else: the tag becomes the whole key,
unique per key just as the untagged key was, so it spreads no better, and it
cannot make a multi-key batch land on one node. A key that already carries a
valid tag is rendered identically on either setting, so a caller that places its
own tag keeps the slot it chose with or without the flag -- the flag makes no
difference to batching in either direction. What it does do for a brace-free key
is make its slot independent of everything rendered in front of it, so one key
lands on one slot across every cache rendering through
DefaultRedisKeyStrategyFactory -- ICache's 's', IHashCache's 'h',
RedisSetCache's 'se', the distributed adapter's 'dh', and any differentiator
passed to its string overload -- and across apps with a different AppShortName.
Two shapes fall outside that: a key whose braces form no valid tag, or that is
empty, is refused outright when the flag is set (#177); and AppShortName and
Separator permit braces, which supply the first tag and override the key's own,
since the strategy validates only the key. The docs say so now rather than
promising the general case.
What does matter on a cluster is unconditional already: a caller's own tag
survives, and a cross-slot batch is refused with a message naming the two keys
that disagree. settings.md told readers to "enable for Redis Cluster deployments
that span multiple shards", which the flag does not deliver; it, the how-to, the
sample settings and the sample README now say what it actually does.
Still read, because a deployment that set it would relocate every entry with a
brace-free key if it stopped being honored; the factory suppresses its own
warning for that reason.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01NsPw6MZHPGmpbo6WDuLzF1
Signed-off-by: Cosmin Staicu <cosmin.staicu@uipath.com>
60f9996 to
ba6709d
Compare
Follow-up to #177. Docs and an
[Obsolete]only — no behaviour change.Why
CacheOptions.ShardKeyEnabledwraps an untagged cache key in a{...}hash tag so the slot follows the key rather thanAppShortNameand the keyspace. It promises cluster sharding and delivers almost none of it.It does not improve distribution. The tag it adds is the whole key, so it is unique per key and CRC16 spreads it exactly as the untagged key would.
It cannot make a multi-key batch single-slot. Same reason — every key gets its own tag, so
GetAsync(CacheKey[])is cross-slot with the flag on just as without it.It makes no difference even to a caller who tags their own keys. Take
{org1}:aand{org1}:b:ShardKeyEnabled=trueapp:s:{org1}:a— valid tag, left aloneorg1ShardKeyEnabled=falseapp:s:{org1}:a— prefix joinedorg1Identical. So the flag is a no-op for batching in both directions.
What it actually does is co-locate the same key across the
sandhkeyspaces and across apps with a differentAppShortName. Neither is something anyone asked for, and the second is what prompted the question that led here.Meanwhile everything that matters on a cluster is already unconditional:
None of those consult
ShardKeyEnabled.What changed
[Obsolete]on the property, with a message saying what it really does and that flipping it relocates every entry.DefaultRedisKeyStrategyFactorysuppresses its ownCS0618. The flag is still honored — a deployment that set it would relocate every key if it stopped being read.docs/reference/settings.mdno longer tells readers to "enable for Redis Cluster deployments that span multiple shards", and points them at giving their keys a shared hash tag instead.docs/how-to/telemetry-and-strategies.mdno longer describes it as handling "shard-key routing automatically".### Deprecatedentry in the changelog.ShardPrefixRedisKeyStrategystays as it is: it is public API, it is what the still-honored flag selects, and a customIRedisKeyStrategyFactorycan use it deliberately. It would go with the flag in a future major, not here.Tests
Unchanged and green — net10.0 1704 pass, net8.0 1683 pass, 0 failures (Redis integration tests skipped, no Docker). Build clean at 0 warnings with the suppression scoped to the one line that reads the flag.
🤖 Generated with Claude Code
https://claude.ai/code/session_01NsPw6MZHPGmpbo6WDuLzF1