Skip to content

Add a unique ID to Interval properties, allow multiple intervals with the same (start, end)#6407

Merged
pleath merged 5 commits into
mainfrom
user/pleath/duplicate-ranges
Jul 9, 2021
Merged

Add a unique ID to Interval properties, allow multiple intervals with the same (start, end)#6407
pleath merged 5 commits into
mainfrom
user/pleath/duplicate-ranges

Conversation

@pleath
Copy link
Copy Markdown
Contributor

@pleath pleath commented Jun 10, 2021

Add methods to get and remove an interval from a collection by ID. Use ID as a third key to determine order in cases of identical (start, end). "delete(start, end)" will remove all (start, end) intervals from the collection. Each such delete will be serialized as an independent operation.

@github-actions github-actions Bot added area: dds Issues related to distributed data structures area: dds: sharedstring area: tests Tests to add, test infrastructure improvements, etc labels Jun 10, 2021
@pleath
Copy link
Copy Markdown
Contributor Author

pleath commented Jun 10, 2021

Replaces #6279.

@msfluid-bot
Copy link
Copy Markdown
Collaborator

msfluid-bot commented Jun 10, 2021

@fluidframework/base-host: No change
Metric NameBaseline SizeCompare SizeSize Diff
main.js 178.64 KB 178.64 KB No change
Total Size 178.64 KB 178.64 KB No change
@fluid-example/bundle-size-tests: +1.83 KB
Metric NameBaseline SizeCompare SizeSize Diff
container.js 150.96 KB 150.96 KB No change
map.js 48.69 KB 48.69 KB No change
matrix.js 147.17 KB 147.33 KB +172 Bytes
odspDriver.js 197.23 KB 197.23 KB No change
odspPrefetchSnapshot.js 28.17 KB 28.17 KB No change
sharedString.js 163.55 KB 165.21 KB +1.66 KB
Total Size 735.76 KB 737.59 KB +1.83 KB

Baseline commit: eec1d66

Generated by 🚫 dangerJS against ac0e1c1

@pleath
Copy link
Copy Markdown
Contributor Author

pleath commented Jun 10, 2021

Addresses #6108, #6109.

@pleath pleath force-pushed the user/pleath/duplicate-ranges branch 2 times, most recently from b3b85e1 to aa65640 Compare June 15, 2021 19:16
Comment thread packages/dds/sequence/src/intervalCollection.ts Outdated
@anthony-murphy
Copy link
Copy Markdown
Contributor

    it("propagates", async () => {

can you add some multi client tests


Refers to: packages/test/test-end-to-end-tests/src/test/sharedInterval.spec.ts:337 in aa65640. [](commit_id = aa656409b2466d834de50a8fbad920c314eec614, deletion_comment = False)

@anthony-murphy
Copy link
Copy Markdown
Contributor

a.addPropertySet(b.properties);

how does this, defaultIntervalConflictResolver behave with local vs remote conflict? do both clients end up with the same id, or will different id's never conflict


Refers to: packages/dds/sequence/src/intervalCollection.ts:288 in aa65640. [](commit_id = aa656409b2466d834de50a8fbad920c314eec614, deletion_comment = False)

@anthony-murphy
Copy link
Copy Markdown
Contributor

    it("propagates", async () => {

it would be good to add some scenario focus tests. currently you really only test the local client, as there is only one container, and you never load a new container to test rehydration


In reply to: 861874250


Refers to: packages/test/test-end-to-end-tests/src/test/sharedInterval.spec.ts:337 in aa65640. [](commit_id = aa656409b2466d834de50a8fbad920c314eec614, deletion_comment = False)

Comment thread packages/dds/sequence/src/intervalCollection.ts Outdated
@pleath pleath force-pushed the user/pleath/duplicate-ranges branch from aa65640 to 811a03d Compare June 15, 2021 23:24
@github-actions github-actions Bot requested a review from anthony-murphy June 15, 2021 23:24
@pleath
Copy link
Copy Markdown
Contributor Author

pleath commented Jun 16, 2021

#6407 (comment)

I do see a client receiving the relevant ops in the test I've been adding to. At least the non-local add(serialized) and delete(serialized) getting called to receive the ops. Can you point me to an example of code that loads a new container to test rehydration?

@pleath
Copy link
Copy Markdown
Contributor Author

pleath commented Jun 16, 2021

#6407 (comment)

Yeah, different ID's won't conflict. The compare function will compare ID's if (start, end) are the same.

Comment thread packages/dds/sequence/src/intervalCollection.ts Outdated
@pleath pleath force-pushed the user/pleath/duplicate-ranges branch from 97394fe to da7613e Compare June 22, 2021 21:11
@github-actions github-actions Bot requested a review from anthony-murphy June 22, 2021 21:11
@pleath pleath force-pushed the user/pleath/duplicate-ranges branch from da7613e to a1d7da7 Compare June 22, 2021 21:20
Comment thread packages/dds/sequence/src/intervalCollection.ts Outdated
Comment thread packages/dds/sequence/src/intervalCollection.ts Outdated
Copy link
Copy Markdown
Contributor

@anthony-murphy anthony-murphy Jun 23, 2021

Choose a reason for hiding this comment

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

sequenceNumber

we would usually call this referenceSequenceNumber, as sequenceNumbers are provided by the server #WontFix

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

looks to be pre-existing

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Do you think the member name in ISerializedInterval should be changed?

Copy link
Copy Markdown
Contributor

@anthony-murphy anthony-murphy Jun 23, 2021

Choose a reason for hiding this comment

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

addInternal

should the above add method call this #Closed

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

looks like you moved some methods which make the merge gross, but i'm not sure why anything in add needed to change

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

add used to call addInternal, which would hit the case below

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I changed it to make the local and non-local cases more distinct. I had to do that for delete. It does make debugging easier if addInternal is only called in the case where an op comes over the wire. Sorry about the merge, though.

Comment thread packages/dds/sequence/src/intervalCollection.ts Outdated
Comment thread packages/dds/sequence/src/intervalCollection.ts Outdated
@pleath pleath force-pushed the user/pleath/duplicate-ranges branch from a1d7da7 to 900ac0b Compare June 24, 2021 00:49
@github-actions github-actions Bot requested a review from anthony-murphy June 24, 2021 00:49
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

it doesn't actually work this way. you need to actually set the reserved id field in props on load from the snapshot, as in the case of shared inverval on a shared string the endpoints can change over time as modifications happen.

overall, i think we might be tackling too many things is this pr, which is making it hard to get any of them in.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Updating to handle the load case, which is the one I wasn't handling.

@pleath pleath force-pushed the user/pleath/duplicate-ranges branch from 900ac0b to 73740d3 Compare June 25, 2021 00:42
@github-actions github-actions Bot requested a review from anthony-murphy June 25, 2021 00:43
@pleath pleath force-pushed the user/pleath/duplicate-ranges branch 2 times, most recently from 7c17683 to 73a0bed Compare June 29, 2021 00:05
Copy link
Copy Markdown
Contributor

@anthony-murphy anthony-murphy left a comment

Choose a reason for hiding this comment

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

:shipit:

@pleath pleath force-pushed the user/pleath/duplicate-ranges branch from c59d1c2 to 21fcf43 Compare July 8, 2021 00:12
@github-actions github-actions Bot requested a review from anthony-murphy July 8, 2021 00:12
pleath added 5 commits July 9, 2021 12:03
Add and delete ops will be supplied with ID's. delete(start, end) will be deprecated; for now it will delete the interval with a legacy ID based on start/end.
Use the interval ID as a third key for sorting. Fix an issue in serialization of add ops that was causing the original ID not to be sent over the wire.
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 area: tests Tests to add, test infrastructure improvements, etc

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Shared Interval: Functionality: Support multiple intervals on a range Shared Interval: Programmability: get and remove interval based on an id

3 participants