Conversation
Contributor
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Unresolved moderate issues remain in option resolution, migration reliability and TTL preservation, and legacy-state tracking.
Review effort: Lite
Findings: 2
Open (3)
What changed in this PR
Updates DynamoDB grain storage to default empty ServiceId values to ClusterOptions.ServiceId, detect legacy state, and support migration.
Changes:
- Adds legacy-key detection, migration, and startup safeguards.
- Adds configuration options, binding, scan support, API updates, and documentation.
- Adds key-format and migration tests.
| File | Reviewed changes and final findings |
|---|---|
test/Extensions/Orleans.AWS.Tests/StorageTests/DynamoDBGrainStorageKeyFormatTests.cs |
Covers key formats and migration behavior. |
src/AWS/Shared/Storage/DynamoDBStorage.cs |
Adds single-page table scanning. |
src/AWS/Orleans.Persistence.DynamoDB/Provider/DynamoDBGrainStorage.cs |
Moderate (3 votes): retain migration markers until success. Moderate (1 vote): preserve legacy TTL. Moderate (1 vote): prevent unbounded legacy-read tracking. |
src/AWS/Orleans.Persistence.DynamoDB/Options/DynamoDBStorageOptions.cs |
Moderate (3 votes): avoid requiring global ClusterOptions when resolving named options. |
src/AWS/Orleans.Persistence.DynamoDB/Hosting/DynamoDBGrainStorageServiceCollectionExtensions.cs |
Moderate (1 vote): preserve keyed overrides while tolerating missing global ClusterOptions. |
src/AWS/Orleans.Persistence.DynamoDB/Hosting/DynamoDBGrainStorageProviderBuilder.cs |
Nit (2 votes): add configuration-binding tests for the new options, including invalid values. |
src/api/AWS/Orleans.Persistence.DynamoDB/Orleans.Persistence.DynamoDB.cs |
Updates the public API surface. |
docs/site/src/content/docs/grains/grain-persistence/dynamodb-storage.md |
Documents defaults and migration behavior. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Laurianti
force-pushed
the
dynamodb-service-id-default
branch
from
September 26, 2026 10:08
5c82f16 to
f355054
Compare
…expiry, and check the current key on clear
Laurianti
force-pushed
the
dynamodb-service-id-default
branch
from
September 26, 2026 11:58
f355054 to
43107d4
Compare
…cide again after losing it
Laurianti
force-pushed
the
dynamodb-service-id-default
branch
from
September 26, 2026 12:55
43107d4 to
9f1007c
Compare
Laurianti
force-pushed
the
dynamodb-service-id-default
branch
from
September 26, 2026 14:04
9f1007c to
7e3a7cd
Compare
…the current one is deleted on clear
Laurianti
force-pushed
the
dynamodb-service-id-default
branch
from
September 26, 2026 17:19
7e3a7cd to
0babd4b
Compare
…retrying its write for ever
Laurianti
force-pushed
the
dynamodb-service-id-default
branch
from
September 26, 2026 19:48
2c6d6c5 to
877471d
Compare
…ate read from the current key while migrating
Laurianti
force-pushed
the
dynamodb-service-id-default
branch
from
September 26, 2026 22:58
877471d to
f4a861b
Compare
Laurianti
force-pushed
the
dynamodb-service-id-default
branch
from
September 27, 2026 07:05
f4a861b to
34d0a6b
Compare
Laurianti
force-pushed
the
dynamodb-service-id-default
branch
from
September 27, 2026 07:39
34d0a6b to
4115a7a
Compare
…k state, and log the chosen format
Laurianti
force-pushed
the
dynamodb-service-id-default
branch
from
October 3, 2026 08:09
4115a7a to
b13002e
Compare
…tart over unmigrated empty-ServiceId state
Laurianti
force-pushed
the
dynamodb-service-id-default
branch
from
October 3, 2026 11:46
b13002e to
06d53ff
Compare
This branch has not been deployed
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.
Builds on #11352, whose commits this branch includes; the changes of this PR alone: Laurianti/orleans@dynamodb-service-id...dynamodb-service-id-default
Problem
With #11352, an empty
DynamoDBStorageOptions.ServiceIdstill stays empty unlessUseClusterServiceIdis set, so a new deployment still gets keys starting with an underscore, unlike every other grain storage provider.Solution
UseClusterServiceIdleft unset now meansClusterOptions.ServiceId. This is a breaking change for deployments that never setServiceId, and it is made so that it cannot hide their state: when the table records the emptyServiceIdor holds state written with it, the provider fails to start, with a message that names both ways forward.UseClusterServiceIdunsetfalsetrueClusterServiceIdorMigratingToClusterServiceIdClusterOptions.ServiceId; a recorded migration goes onClusterOptions.ServiceIdEmptyServiceIdServiceId, with a warningClusterOptions.ServiceId, and withMigrateLegacyKeysthe state is movedClusterOptions.ServiceIdServiceId, with a warningClusterOptions.ServiceIdServiceId, with a warningClusterOptions.ServiceId, and withMigrateLegacyKeysthe state is movedA table that records
EmptyServiceIdstops the provider even when it holds no such state yet: the silos that recorded it, running #11352, may still be writing those keys, and no scan sees what they write after it. For a table without a record, the provider scans for keys starting with an underscore, other than the key format record, with consistent reads in pages of 100, stopping at the first match (AnyAsync, a new internal method of the sharedDynamoDBStorage). A table without such keys is read in full once: the record written afterwards skips the scan on the next starts. State written with an explicitServiceIddoes not stop the provider, unless thatServiceIdstarts with an underscore, since the two cannot be told apart; the docs say to setUseClusterServiceIdexplicitly then. A silo from before #11352 neither reads nor writes the key format record, so an upgrade straight from such a version stops those silos first or setsUseClusterServiceIdexplicitly; the docs say so.The startup warning now appears only when
UseClusterServiceIdisfalse, and no longer announces a future default.An explicit
ServiceIdworks as before.Fixes: #9971