Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix Noops #11790

Merged
merged 5 commits into from Sep 2, 2022
Merged

Fix Noops #11790

merged 5 commits into from Sep 2, 2022

Conversation

vladsud
Copy link
Contributor

@vladsud vladsud commented Sep 1, 2022

Splitting noop logic out of https://github.com/microsoft/FluidFramework/pull/11262/files

Through code review, I've noticed that we send content="null" for non-immediate noops, but service expects content === null to differentiate immediate vs. non-immediate noops. In other words, non-immediate noops are considered to be immediate noops by service and they are never coalesced! I've noticed it only because I worked (not part of this PR) to move content strigification out of loader layer into runtime.

While we can fix it on service side (and leave client as is), I really do not like current design and special types based on type of content. So splitting noops into "accept" (formerly - immediate noops) and noops (non-immediate). Once this change propagates through the system, service can

  • coalesce ALL noops.
  • service may choose to do some special treatment of accept ops, though I do not see any reason for that. For example, today service will drop it if such op does not change MSN. Given they are used only for acceptance of code proposals (which are very rare), I'd suggest simplifying service logic and not have any handling for it.

@vladsud vladsud requested review from a team as code owners September 1, 2022 22:17
@github-actions github-actions bot added area: loader Loader related issues area: runtime Runtime related issues area: tests Tests to add, test infrastructure improvements, etc public api change Changes to a public API labels Sep 1, 2022
@vladsud vladsud mentioned this pull request Sep 1, 2022
@github-actions github-actions bot added the base: main PRs targeted against main branch label Sep 1, 2022
@msfluid-bot
Copy link
Collaborator

msfluid-bot commented Sep 1, 2022

@fluid-example/bundle-size-tests: +97 Bytes
Metric NameBaseline SizeCompare SizeSize Diff
aqueduct.js 391.97 KB 391.96 KB -9 Bytes
connectionState.js 680 Bytes 680 Bytes No change
containerRuntime.js 190.95 KB 190.94 KB -9 Bytes
loader.js 151.71 KB 151.81 KB +103 Bytes
map.js 42.63 KB 42.63 KB No change
matrix.js 131.62 KB 131.62 KB No change
odspDriver.js 150.23 KB 150.24 KB +6 Bytes
odspPrefetchSnapshot.js 38.39 KB 38.4 KB +6 Bytes
sharedString.js 152.92 KB 152.92 KB No change
Total Size 1.25 MB 1.25 MB +97 Bytes

Baseline commit: ca9df05

Generated by 🚫 dangerJS against 5e7d437

Copy link
Contributor

@andre4i andre4i left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The change is ok but it needs to have the package.json breaking stuff and also in the next branch

@vladsud vladsud merged commit a232bb2 into microsoft:main Sep 2, 2022
@vladsud vladsud deleted the OpProcessing2 branch September 2, 2022 20:04
@github-actions
Copy link
Contributor

github-actions bot commented Sep 2, 2022

This commit is queued for merging with the next branch! Please ignore this PR for now. Contact @microsoft/fluid-cr-infra for help.

vladsud added a commit that referenced this pull request Sep 2, 2022
Introducing new type of messages - "accept", that will be used to replace immediate noops.
This is integral part of PR #11790
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: loader Loader related issues area: runtime Runtime related issues area: tests Tests to add, test infrastructure improvements, etc base: main PRs targeted against main branch public api change Changes to a public API
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants