ConsensusQueue: acquire/release semantics, handle support by vladsud · Pull Request #1395 · microsoft/FluidFramework

vladsud · 2020-03-01T20:26:31Z

Note: I like add/acquire/release naming.
I'm not sure if "complete" is the best term to use.

Support for handles.

curtisman · 2020-03-04T02:37:26Z

+ * Helper method to acquire and complete an item
+ * Should be used in test code only
+ */
+export async function acquireAndComplete<T>(collection: IConsensusOrderedCollection<T>): Promise<T | undefined> {


Are these useful outside of test code? If so, they should be members. #Resolved

And if they are truly test utilities move it to a separate file and have test file import them directory.
That way they don't get exported by the package thru index.ts

In reply to: 387418180 [](ancestors = 387418180)

I moved them to separate file (testUtils), but I need to export them for end-to-end test (or make a separate package, which I do not want :))

In reply to: 387430221 [](ancestors = 387430221,387418180)

curtisman · 2020-03-04T02:46:56Z

+
+interface IConsensusOrderedCollectionAcquireOperation {
+    opName: "acquire";
+    id: string;


May be more explicit name? acquireId? #Resolved

Also tsdoc for the fields.

In reply to: 387420693 [](ancestors = 387420693)

curtisman · 2020-03-04T02:58:20Z

+            value: valueSer,
        };
-        return this.submit(op);
+        await this.submit(op);


You can just return the Promise directly, instead of having an extra await? #WontFix

Can't due to differences in types (void vs. IConsensusOrderedCollectionValue | undefined)

In reply to: 387424473 [](ancestors = 387424473)

curtisman · 2020-03-04T03:01:47Z

        while (true) {
            if (this.data.size() === 0) {
-                // Wait for new entry before trying to remove again
-                await new Promise((resolve, reject) => {


I am wondering whether we might want to reject when the container is closing.

(Side note: we should review what each DDS behavior is like when the container is closing or closed) #Resolved

Yes, we should - good catch.
I do not think we propagate close event down, so I'll open an issue to follow up.

In reply to: 387425273 [](ancestors = 387425273)

#1420

In reply to: 387862465 [](ancestors = 387862465,387425273)

curtisman · 2020-03-04T03:08:04Z

+                opName: "complete",
+                id,
+            };
+            await this.submit(work);


If we disconnected and then reconnected again, we will still send out an op for complete.
Should we return an error to indicate that "complete" isn't successful? #Resolved

I've added proper events here, but I'm adding one more to make it easier to track transitions - localRelease.
There is nowhere to return an error, as completion is requested through returning ConsensusResult.Complete from callback.
I want to take an approach of seeing an actual case where eventing is not enough / not convenient before changing (or adding) API here

In reply to: 387426675 [](ancestors = 387426675)

curtisman · 2020-03-04T03:13:01Z

        assert(message.contents.opName === pending.message.opName);
-        assert(message.clientSequenceNumber === -1
-            || message.clientSequenceNumber === pending.clientSequenceNumber);
+        assert(message.clientSequenceNumber === pending.clientSequenceNumber);


I believe the -1 check might necessary for messages that are resent during disconnect? #ByDesign

clientSequenceNumber should never be -1.
Resend is done after catching up (with new clientID) and it just starts counting over.

init / reconnect:
this.clientSequenceNumber = 0;

Sending ops:
const message: IDocumentMessage = {
clientSequenceNumber: ++this.clientSequenceNumber,

In reply to: 387427831 [](ancestors = 387427831)

curtisman · 2020-03-04T03:21:22Z

});

acquire and release end to end tests? #Resolved

Refers to: packages/test/end-to-end/src/test/consensusOrderedCollectionEndToEndTests.spec.ts:296 in 1f88d7b. [](commit_id = 1f88d7b, deletion_comment = False)

…into QueueAcquireRelease # Conflicts: # packages/runtime/consensus-ordered-collection/package.json

vladsud · 2020-03-04T18:51:05Z

});

Do you mean name of file / name of tests, or there are some cases missing in terms of coverage?

In reply to: 594304592 [](ancestors = 594304592)

Refers to: packages/test/end-to-end/src/test/consensusOrderedCollectionEndToEndTests.spec.ts:296 in 1f88d7b. [](commit_id = 1f88d7b, deletion_comment = False)

vladsud · 2020-03-04T19:28:59Z

});

"Can add and release data" test covers it in package itself.
Adding equivalent case here as well.

In reply to: 594747266 [](ancestors = 594747266,594304592)

Refers to: packages/test/end-to-end/src/test/consensusOrderedCollectionEndToEndTests.spec.ts:296 in 1f88d7b. [](commit_id = 1f88d7b, deletion_comment = False)

curtisman

…into QueueAcquireRelease

vladsud added 2 commits February 29, 2020 22:37

Initial implementaiton

0bf66a3

All UTs passing.

efa77e2

Support for handles.

vladsud requested review from curtisman, skylerjokiel and tanviraumi March 1, 2020 20:26

lint

1f88d7b

curtisman reviewed Mar 4, 2020

View reviewed changes

vladsud added 2 commits March 4, 2020 09:26

Merge branch 'master' of https://github.com/microsoft/FluidFramework …

02b744f

…into QueueAcquireRelease # Conflicts: # packages/runtime/consensus-ordered-collection/package.json

Fix merge conflict

1c594fb

PR feedback

32fd776

curtisman approved these changes Mar 4, 2020

View reviewed changes

vladsud added 2 commits March 4, 2020 16:51

docs

cbc33a2

Merge branch 'master' of https://github.com/microsoft/FluidFramework …

d3e2fd4

…into QueueAcquireRelease

vladsud merged commit f844fee into microsoft:master Mar 5, 2020

vladsud deleted the QueueAcquireRelease branch March 13, 2020 15:24

This was referenced May 27, 2026

feat(ordered-collection): implement reSubmitSquashed hook (identity transform) #27407

Closed

perf(ordered-collection): back SnapshotableQueue with DoublyLinkedList #27421

Draft

Conversation

vladsud commented Mar 1, 2020

Uh oh!

curtisman Mar 4, 2020 • edited by vladsud Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

curtisman Mar 4, 2020

Choose a reason for hiding this comment

Uh oh!

vladsud Mar 4, 2020

Choose a reason for hiding this comment

Uh oh!

curtisman Mar 4, 2020 • edited by vladsud Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

curtisman Mar 4, 2020

Choose a reason for hiding this comment

Uh oh!

curtisman Mar 4, 2020 • edited by vladsud Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vladsud Mar 4, 2020

Choose a reason for hiding this comment

Uh oh!

curtisman Mar 4, 2020 • edited by vladsud Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vladsud Mar 4, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vladsud Mar 4, 2020

Choose a reason for hiding this comment

Uh oh!

curtisman Mar 4, 2020 • edited by vladsud Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vladsud Mar 4, 2020

Choose a reason for hiding this comment

Uh oh!

curtisman Mar 4, 2020 • edited by vladsud Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vladsud Mar 4, 2020

Choose a reason for hiding this comment

Uh oh!

curtisman commented Mar 4, 2020 • edited by vladsud Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vladsud commented Mar 4, 2020

Uh oh!

vladsud commented Mar 4, 2020

Uh oh!

curtisman left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

curtisman Mar 4, 2020 •

edited by vladsud

Loading

curtisman Mar 4, 2020 •

edited by vladsud

Loading

curtisman Mar 4, 2020 •

edited by vladsud

Loading

curtisman Mar 4, 2020 •

edited by vladsud

Loading

vladsud Mar 4, 2020 •

edited

Loading

curtisman Mar 4, 2020 •

edited by vladsud

Loading

curtisman Mar 4, 2020 •

edited by vladsud

Loading

curtisman commented Mar 4, 2020 •

edited by vladsud

Loading