Move drivers to streaming API for op retrieval #5703

vladsud · 2021-04-01T23:39:27Z

Hide the logic on how driver fetches ops behind streaming API.

That ensures that driver controls such aspects as

how ops are retrieved - in batches, or though http streaming
If batches are used, driver controls batch sizes and any concurrency.

As part of redesign, start introducing AbortSignal on all of our network APIs.
This allows caller to cancel complex operations. In future that would likely need to change as well to include more scenarios (including progress reporting, prioritization, etc.).

Also exposing cachedOnly flag on request for loader to have ability (future PRs) to request cached ops (cached in snapshot and cached on local machine) to be able to rehydrate container to more recent state without waiting for network call to come back. This will ensure we could address gaps in user experience.

Making [from, to) arguments to be inclusive on left side and exclusive on right side - much easier to think and do math (things like calculate length, next batch, etc.)

Expose overrides in ODSP driver, such that fetch tool can continue to be more aggressive (currently - batches of 20K in size and 4 concurrent requests).

Areas of most feedback I'd love to get - naming :)

IStream
readMessages() (more unique name, easier to find)
emptyMessageStream
streamFromMessages

Unrelated (sort of) changes:

Address an issue in tests where driver & loader versions were mismatched. For now, they are one and the same. In future, this should be controlled through compat configs, not individual tests decided what version to use
Catch up logic that our tests were using does not really work (or works by chance) due to r11s driver not supporting checkpoint sequence numbers. Make wait logic more inclusive by relying on own join op as a barrier.

…more predictable catch up

packages/loader/driver-definitions/src/storage.ts

…switch to async iterator in the future

curtisman · 2021-04-05T22:38:55Z

 * Retrieves all the delta operations within the inclusive sequence number range

Update comment, and include description per param

Refers to: packages/loader/driver-definitions/src/storage.ts:47 in 2f1834d. [](commit_id = 2f1834d, deletion_comment = False)

server/routerlicious/packages/lambdas/src/scribe/interfaces.ts

packages/test/local-server-tests/src/test/noDeltaStream.spec.ts

packages/drivers/odsp-driver/src/odspDeltaStorageService.ts

curtisman · 2021-04-05T23:50:52Z

packages/loader/driver-definitions/src/storage.ts

+        to: number | undefined,
+        abortSignal?: AbortSignal,
+        cachedOnly?: boolean,
+    ): IStream<ISequencedDocumentMessage[]>;


Should be make this an "async iterator" since they are so similar already?

I thought about it. Maybe. I have a feeling that it will be useful to be able to query it for more things. Like, have progress reporting (i.e. how many ops are still there, if that can be answered). Maybe we can have it both?
I do not have strong opinion here, happy to convert to async iterator.

The part that worries me is red X next to Safari on iOS:
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Symbol/asyncIterator
Can't find equivalent, but I had to back out code earlier due to usage of async generators due to compat concerns, so I'm a bit hesitant to jump here.

I've done manual test on iOS and async iterator is working fine for me, being on latest iOS.
That said, https://caniuse.com/?search=async%20iterator also is in red. So I'm not sure.
I'm thinking of leaving it as is for now (though ask around), until it becomes green across the board (yes, future changes would require back compat, but it's not such a big deal imho). Let me know what you think

curtisman · 2021-04-05T23:54:24Z

while (signal?.aborted !== true) {

Should we throw an error if it is aborted?

Refers to: packages/loader/driver-utils/src/parallelRequests.ts:362 in 2f1834d. [](commit_id = 2f1834d, deletion_comment = False)

…to FetchOpsDriver # Conflicts: # packages/drivers/odsp-driver/src/contracts.ts # packages/drivers/odsp-driver/src/odspDeltaStorageService.ts # packages/drivers/odsp-driver/src/odspDocumentService.ts

vladsud · 2021-04-06T18:24:25Z

Ping. I'd love to move forward with it.

anthony-murphy · 2021-04-07T00:04:53Z

server/routerlicious/packages/routerlicious-base/src/alfred/routes/api/deltas.ts

@@ -24,7 +24,7 @@ import winston from "winston";
 import { IAlfredTenant } from "@fluidframework/server-services-client";
 import { Constants } from "../../../utils";

-export async function getDeltas(
+async function getDeltas(


why the server changes?

anthony-murphy

looks good. we'll just need to pay attention to rollout, as this breaks driver/loader contracts which could cause problems with dynamic drivers once that lands

…to FetchOpsDriver # Conflicts: # packages/test/test-drivers/src/odspDriverApi.ts # packages/test/test-end-to-end-tests/src/test/contextReload.spec.ts

msfluid-bot · 2021-04-08T03:24:50Z

⯆ @fluidframework/base-host: -4.59 KB

Metric Name	Baseline Size	Compare Size	Size Diff
main.js	178.76 KB	174.17 KB	⯆ -4.59 KB
Total Size	178.76 KB	174.17 KB	⯆ -4.59 KB

⯅ @fluid-example/bundle-size-tests: +5.24 KB

Metric Name	Baseline Size	Compare Size	Size Diff
container.js	201.78 KB	201.78 KB	■ No change
map.js	49.11 KB	49.11 KB	■ No change
matrix.js	143.42 KB	143.42 KB	■ No change
odspDriver.js	203.23 KB	208.47 KB	⯅ +5.24 KB
sharedString.js	158.14 KB	158.14 KB	■ No change
Total Size	755.69 KB	760.93 KB	⯅ +5.24 KB

Baseline commit: 6555aa8

Generated by 🚫 dangerJS against db0e0dd

vladsud added 6 commits March 31, 2021 16:39

Compile

6c27ff9

Argument position, +1 logic

675cd46

Cached mode

e647309

ops batching policies for fetch-tool

4aee8ce

Renames, add cancelation

ab7f52c

Address compatibility tests: driver to be on same version as loader; …

f8399e3

…more predictable catch up

vladsud requested review from anthony-murphy, tanviraumi, curtisman and jatgarg April 1, 2021 23:39

github-actions bot requested review from znewton, markfields, agarwal-navin, GaryWilber and hedasilv April 1, 2021 23:39

Fix flaky tests

1b4fb33

anthony-murphy reviewed Apr 2, 2021

View reviewed changes

packages/loader/driver-definitions/src/storage.ts Outdated Show resolved Hide resolved

anthony-murphy reviewed Apr 2, 2021

View reviewed changes

packages/loader/driver-definitions/src/storage.ts Show resolved Hide resolved

Renames, remove cancel(), move closer to iterator shape of result to …

2f1834d

…switch to async iterator in the future

github-actions bot requested a review from anthony-murphy April 2, 2021 21:36

curtisman reviewed Apr 5, 2021

View reviewed changes

server/routerlicious/packages/lambdas/src/scribe/interfaces.ts Outdated Show resolved Hide resolved

curtisman reviewed Apr 5, 2021

View reviewed changes

packages/test/local-server-tests/src/test/noDeltaStream.spec.ts Show resolved Hide resolved

curtisman reviewed Apr 5, 2021

View reviewed changes

packages/drivers/odsp-driver/src/odspDeltaStorageService.ts Show resolved Hide resolved

curtisman reviewed Apr 5, 2021

View reviewed changes

vladsud added 2 commits April 5, 2021 20:12

PR feedback

6b2ba77

anthony-murphy reviewed Apr 7, 2021

View reviewed changes

anthony-murphy approved these changes Apr 7, 2021

View reviewed changes

vladsud mentioned this pull request Apr 8, 2021

We attempt to upload summary more often than we generate it, suggesting retry logic kicks in when summarizer is offline and already gone #5751

Closed

vladsud added 3 commits April 7, 2021 19:04

Fix build

3c3f244

Merge branch 'main' of https://github.com/Microsoft/FluidFramework in…

4b141e3

…to FetchOpsDriver # Conflicts: # packages/test/test-drivers/src/odspDriverApi.ts # packages/test/test-end-to-end-tests/src/test/contextReload.spec.ts

Incorrect merge: file was deleted, but left around in local branch

db0e0dd

vladsud merged commit d4f68c8 into microsoft:main Apr 8, 2021

vladsud deleted the FetchOpsDriver branch April 8, 2021 06:15

vladsud mentioned this pull request Apr 8, 2021

Parallel request policy for DeltaManager / move to streaming API #5523

Closed

github-actions bot requested a review from anthony-murphy April 8, 2021 07:54

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move drivers to streaming API for op retrieval #5703

Move drivers to streaming API for op retrieval #5703

vladsud commented Apr 1, 2021

curtisman commented Apr 5, 2021

curtisman Apr 5, 2021

vladsud Apr 6, 2021

vladsud Apr 8, 2021

curtisman commented Apr 5, 2021

vladsud commented Apr 6, 2021

anthony-murphy Apr 7, 2021

anthony-murphy left a comment

msfluid-bot commented Apr 8, 2021

Move drivers to streaming API for op retrieval #5703

Move drivers to streaming API for op retrieval #5703

Conversation

vladsud commented Apr 1, 2021

curtisman commented Apr 5, 2021

curtisman Apr 5, 2021

Choose a reason for hiding this comment

vladsud Apr 6, 2021

Choose a reason for hiding this comment

vladsud Apr 8, 2021

Choose a reason for hiding this comment

curtisman commented Apr 5, 2021

vladsud commented Apr 6, 2021

anthony-murphy Apr 7, 2021

Choose a reason for hiding this comment

anthony-murphy left a comment

Choose a reason for hiding this comment

msfluid-bot commented Apr 8, 2021