Enable rollback for ConsensusRegisterCollection #24734
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR enables rollback functionality for ConsensusRegisterCollection so that write operations resolve with false when rolled back or when the container is disposed.
- Updated end-to-end tests to validate rollback and disposal behaviors.
- Modified ConsensusRegisterCollection to use an internal event emitter for ack and rollback resolution of pending messages.
- Updated API documentation to reflect the new sealed rollback method.
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| packages/test/test-end-to-end-tests/src/test/consensusRegisterCollectionEndToEndTests.spec.ts | Added tests for write promise resolution on container disposal and rollback scenarios |
| packages/dds/register-collection/src/consensusRegisterCollection.ts | Implemented rollback handling using an internal emitter and updated the write method to support three resolution paths |
| packages/dds/register-collection/api-report/register-collection.legacy.alpha.api.md | Documented the new rollback method in the API report |
| sharedMap1.set("collection", collection1.handle); | ||
| await provider.ensureSynchronized(); | ||
| const write1P = collection1.write("key1", "value1"); | ||
| if (container1.dispose !== undefined) { |
There was a problem hiding this comment.
[nitpick] Consider checking for the existence of the dispose method using 'typeof container1.dispose === "function"' to ensure the check is robust.
| if (container1.dispose !== undefined) { | |
| if (typeof container1.dispose === "function") { |
Copilot uses AI. Check for mistakes.
| // 2. The write is rolled back | ||
| // 3. The runtime is disposed | ||
| // The boolean value returned by the promise is true if the attempted write was ack'd and won, false otherwise. | ||
| return new Promise<boolean>((resolve) => { |
There was a problem hiding this comment.
[nitpick] Add inline comments explaining the use of pendingMessageId along with the internalEvents listeners in the write method to improve readability and maintainability.
Copilot uses AI. Check for mistakes.
github-actions
bot
added
area: dds
|
🔗 No broken links found! ✅ Your attention to detail is admirable. linkcheck output |
|
i would like to see more test coverage here. can this be integrated into the local server stress? For matrix i added both stress and unit test coverage: #24604 |
| @@ -343,6 +395,16 @@ export class ConsensusRegisterCollection<T> | |||
| return serializer.parse(content); | |||
| } | |||
|
|
|||
| /** | |||
| * @sealed | |||
CRC doesn't actually modify anything until the op is ack'd, so nothing is required to restore data state in rollback.
Only thing is to ensure we resolve the promise in a sane way, where we can use the existing semantic of resolving with
falseto mean "your write didn't go through".Part of AB#31942