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

Notify channel and channel contexts of their used routes. #4764

Merged
merged 3 commits into from
Jan 12, 2021

Conversation

agarwal-navin
Copy link
Contributor

This change notifies channel contexts of used routes. The contexts don't use these routes to update their state but this puts the code in place for that to eventually happen.
When a data store's used routes are updated, if it is realized, it updates the context's used routes. If it is not realized yet, this is updated when the data store is realized.

Note: This is needed to fix the problem in #4750. I will have a PR for that out soon.

@github-actions github-actions bot requested a review from curtisman January 8, 2021 00:40
@msfluid-bot
Copy link
Collaborator

msfluid-bot commented Jan 8, 2021

@fluidframework/base-host: No change
Metric NameBaseline SizeCompare SizeSize Diff
main.js 167.04 KB 167.04 KB No change
Total Size 167.04 KB 167.04 KB No change
@fluid-example/bundle-size-tests: +809 Bytes
Metric NameBaseline SizeCompare SizeSize Diff
container.js 191.41 KB 192.2 KB +809 Bytes
map.js 45.84 KB 45.84 KB No change
matrix.js 144.52 KB 144.52 KB No change
odspDriver.js 192.73 KB 192.73 KB No change
sharedString.js 158.46 KB 158.46 KB No change
Total Size 732.96 KB 733.75 KB +809 Bytes

Baseline commit: 4e06335

Generated by 🚫 dangerJS against 7ef8fce

@@ -562,6 +585,9 @@ export abstract class FluidDataStoreContext extends TypedEventEmitter<IFluidData
// returned in packagePath().
Object.freeze(this.pkg);

// Update the used routes of the channel in case GC was run before we were realized.
this.updateChannelUsedRoutes();
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe the intention is for data store to be able to reason about non-referenced DDSs and be able to purge them.
And I'd guess same for sub-DDS level.
Have you considered the impact of calling it at random times (relative to when summarization / snapshot runs)?
I.e. I'd assume that something like at-mentions data store would not be able to use results of this call if it happens outside of summary processing, as undo/redo could reinsert content that points to same route (in case of at-mentions, it's just a key in DDS - the same key gets reinserted, but deleting it after undo as unused would be incorrect).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As per our discussion offline, it is currently going to be difficult to handle the scenario where data stores start using the result of this call to delete stuff and undo / redo happens. Even if this is tied to summary, it is hard to get this right because of the ordering of the delete op and undo / redo op.
So maybe these data stores should not delete things but only exclude them from search results to begin with.

@github-actions github-actions bot requested review from vladsud and arinwt January 10, 2021 19:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants