Skip to content

Save obliterate sequence number on zero-length obliterates#23011

Merged
titrindl merged 10 commits into
microsoft:mainfrom
titrindl:zero-length-obliterate
Nov 11, 2024
Merged

Save obliterate sequence number on zero-length obliterates#23011
titrindl merged 10 commits into
microsoft:mainfrom
titrindl:zero-length-obliterate

Conversation

@titrindl
Copy link
Copy Markdown
Contributor

@titrindl titrindl commented Nov 6, 2024

This PR fixes how we save the obliterate sequence number on the associated pendingSegmentGroup. Previously, the sequence number would never be set for zero-length obliterates, so we would get stuck considering the obliterate as unsequenced.

This PR also surfaces zero-length obliterates in mergeTreeDeltaCallback as long as the obliterate's endpoints are not equal.

@titrindl titrindl requested a review from a team as a code owner November 6, 2024 19:42
@github-actions github-actions Bot added base: main PRs targeted against main branch area: dds Issues related to distributed data structures labels Nov 6, 2024
Comment thread packages/dds/merge-tree/src/mergeTree.ts Outdated
Comment thread packages/dds/merge-tree/src/mergeTree.ts
Copy link
Copy Markdown
Collaborator

@msfluid-bot msfluid-bot left a comment

Choose a reason for hiding this comment

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

Code Coverage Summary

↑ packages.dds.sequence.src:
Line Coverage Change: 0.02%    Branch Coverage Change: 0.05%
Metric NameBaseline coveragePR coverageCoverage Diff
Branch Coverage 89.48% 89.53% ↑ 0.05%
Line Coverage 89.85% 89.87% ↑ 0.02%

Baseline commit: 5156eb4
Baseline build: 305838
Happy Coding!!

Code coverage comparison check passed!!

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Copilot reviewed 2 out of 2 changed files in this pull request and generated no suggestions.

Comments skipped due to low confidence (3)

packages/dds/merge-tree/src/mergeTree.ts:1253

  • Ensure there are tests that cover scenarios where obliterateInfo is undefined.
if (pendingSegmentGroup.obliterateInfo !== undefined) {

packages/dds/merge-tree/src/mergeTree.ts:2108

  • Ensure there are tests that cover scenarios where the start and end positions or sides are equal.
if (start.pos !== end.pos || start.side !== end.side) {

packages/dds/merge-tree/src/mergeTreeNodes.ts:706

  • The assignment to obliterateInfo.seq has been removed. If obliterateInfo.seq is used elsewhere, this could introduce a bug. Ensure obliterateInfo.seq is set correctly.
moveInfo.movedSeqs[seqIdx] = opArgs.sequencedMessage!.sequenceNumber;

Tip: Copilot only keeps its highest confidence comments to reduce noise and keep you focused. Learn more

@titrindl titrindl enabled auto-merge (squash) November 7, 2024 19:25
@titrindl
Copy link
Copy Markdown
Contributor Author

@anthony-murphy I had to merge the sequence changes into this PR because there were two tests failing on assert 0x2d8.

@msfluid-bot
Copy link
Copy Markdown
Collaborator

@fluid-example/bundle-size-tests: +475 Bytes
Metric NameBaseline SizeCompare SizeSize Diff
aqueduct.js 463.74 KB 463.87 KB +132 Bytes
azureClient.js 562.69 KB 562.74 KB +49 Bytes
connectionState.js 724 Bytes 724 Bytes No change
containerRuntime.js 261.99 KB 262 KB +14 Bytes
fluidFramework.js 426.65 KB 426.66 KB +14 Bytes
loader.js 134.18 KB 134.19 KB +14 Bytes
map.js 42.71 KB 42.71 KB +7 Bytes
matrix.js 148.32 KB 148.36 KB +43 Bytes
odspClient.js 528.43 KB 528.48 KB +49 Bytes
odspDriver.js 97.84 KB 97.86 KB +21 Bytes
odspPrefetchSnapshot.js 42.81 KB 42.83 KB +14 Bytes
sharedString.js 164.17 KB 164.27 KB +104 Bytes
sharedTree.js 417.11 KB 417.11 KB +7 Bytes
Total Size 3.36 MB 3.36 MB +475 Bytes

Baseline commit: 3c27751

Generated by 🚫 dangerJS against 17313f0

@titrindl titrindl merged commit c9697ec into microsoft:main Nov 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area: dds: sharedstring area: dds Issues related to distributed data structures base: main PRs targeted against main branch

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants