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

Deprecate SharedObjectSequenceFactory and SharedNumberSequenceFactory #8526

Closed
anthony-murphy opened this issue Dec 9, 2021 · 11 comments
Closed
Milestone

Comments

@anthony-murphy
Copy link
Contributor

These were experimental, and while functional, their behaviors are non-intuitive, and not very useful. We should migrate existing customers away from them and then deprecate and remove them

Originally posted by @anthony-murphy in #8518 (comment)

@tylerbutler
Copy link
Member

tylerbutler commented Dec 16, 2021

I have some concerns about this. Are we replacing these DDSes with something else? There are, I believe, some use-cases for SharedObjectSequence that aren't readily handled by Map. Before we proceed with removing these, I'd like to have a story for these use cases. I accept that the story might be, "Here's how we think you can solve it with SharedMap."

The use-case I'm thinking of is the one in the Badge data object. It uses a SharedObjectSequence to store the log of changes made to the component -- its history. Relevant code here. Arguably sequence is waaaay more than is needed here, but it still seems like the best fit to me.

  1. Items are only ever inserted at the end of the sequence.
  2. Items in the sequence aren't modified.
  3. The insertion order needs to be tracked.

When I think about using a SharedMap for this scenario, I run into problems... I'll describe them here for the benefit of folks not as familiar with the pitfalls of SharedMap.

Option 1: Store an array (or an object, same problems) in a SharedMap key.

How do I handle simultaneous inserts without losing data? Because the whole array is set by each client, then simultaneous inserts will cause one of the inserts to be overwritten.

Option 2: Store individual items in individual SharedMap cells, using a timestamp as the key. When you want to access the history, enumerate the keys, sort them, then access each item via the key. (You still need a tie-breaker for sorting, I guess. Not sure how that would work.

How do I get a trustworthy timestamp in the browser? Is there something in Fluid that I can use for that purpose? After all, Fluid's underlying invariant is everything is totally ordered. :)

Also, the sorting gets problematic as the list size increases. Even if I only want to show a portion of the history, I have to enumerate the keys even if I only want the most recent N items. So then I think, "OK, well I can cache the order in another Map key..." And then I think, "This seems to be the wrong tool for my problem."

Are there other options?

@tylerbutler
Copy link
Member

RE: "This seems to be the wrong tool for my problem..." An InsertOnlyObjectList DDS would be sufficient for this scenario, so I don't want to imply that we must keep ObjectSequence as-is to address this.

@anthony-murphy
Copy link
Contributor Author

I don't think the current sequences are supportable, or something we want to invest in, given that we should remove them. There may be some legitimate cases that they could cover, but far more often than not people want them to do more than they do today.

@tylerbutler
Copy link
Member

far more often than not people want them to do more than they do today.

Agreed, but I don't think that the solution to that is to remove them. Or rather, to remove them and nothing else. Seems like throwing the bathwater out with the baby, even though the dirty bathwater is still useful. OK, that analogy got away from me... :)

@anthony-murphy
Copy link
Contributor Author

I think we could build an ordered list based on map, and sequential ids that would be a better fit here, but again it would need to be built to solve some of the problem you list above.

I would rather have no solution to a problem than a bad solution. bad solutions just create pain for people using them, and us trying to support them. the current sequences are a bad solution to most problem. it comes down to that i want our things to be a pit of sucess, so as developer fall in they find more useful feature and don't get blocked. the current sequence are pit of failure where developers quickly reach their limits.

@ChumpChief
Copy link
Contributor

I should document some thoughts here too -- I've been thinking about this a bit as I was looking through the sequence/merge-tree package API surface. I'll update this comment as I learn more.

Current usage

SharedNumberSequence

Currently has low usage in our repo:

  • table-document - this is already deprecated, we need a strategy to permanently remove it altogether.
  • replay-tool - just remove once other usages are gone.

I don't think it's widely used outside our repo (except indirectly through table-document), but needs more verification. I also don't think this maps to particularly interesting scenarios for our customers, so probably doesn't need any real substitute provided.

SharedObjectSequence

As Tyler mentions above, there are scenarios using this today in our repo that are not unreasonable, at least in intent. Todo is probably the best example of intent, where it's reasonable to think of a todo list as an ordered sequence of items that supports insertion/deletion at arbitrary positions, reordering, etc. I am interested in putting some thought into what this sort of customer would be advised to do.

I have not yet investigated usage outside our repo.

Path forward and staging

I think it's reasonable to go ahead and mark these as deprecated to discourage new uptake. I'll put together a PR to do this at least.

Prior to total removal I think we should ideally identify candidate work that would fill the scenario gap. But I also agree with Tony that it would be good to aggressively remove the temptation of a dead-end DDS so developers don't fall into that trap. We should discuss more after we've had a chance to do more thorough analysis of the current state of things.

@anthony-murphy
Copy link
Contributor Author

anthony-murphy commented Dec 16, 2021

where it's reasonable to think of a todo list as an ordered sequence of items that supports insertion/deletion at arbitrary positions, reordering, etc.

just a note. the sequences do not support reordering, this is a thing that trips people up, and part to the impetus for deprecation

@tylerbutler
Copy link
Member

tylerbutler commented Dec 17, 2021

where it's reasonable to think of a todo list as an ordered sequence of items that supports insertion/deletion at arbitrary positions, reordering, etc.

just a note. the sequences do not support reordering, this is a thing that trips people up, and part to the impetus for deprecation

Is there anything in the API itself that is misleading, or is this a (very understandable) case of peoples' expectations not being met?

We don't talk about this in the docs AFAIK, which is a huge omission. 😞

@tylerbutler
Copy link
Member

I think it's reasonable to go ahead and mark these as deprecated to discourage new uptake. I'll put together a PR to do this at least.

That's a good pragmatic step, thanks!

@anthony-murphy
Copy link
Contributor Author

anthony-murphy commented Dec 17, 2021

i think the current guidance right now would be to use @fluidframework/matrix as a replacement for an append only list. it has many of the same drawbacks, but i think the structure and api feels more rigid, so hopefully there is less desire or expectation for things like move. , i'd probably use orderSeqeuentially around a insertRow and setCell calls

This comment describes how to implement an ordered list with move using map: #8518 (comment)
Probably be good to get an example that actually does it.

anthony-murphy pushed a commit that referenced this issue Dec 20, 2021
…ost usage of SharedObjectSequence (#8585)

This change is part of #8526

This change marks SharedNumberSequence and SharedObjectSequence as deprecated to discourage new usage, and removes most SharedObjectSequence usage in our repository (replay-tool is left unchanged). It also removes the useSyncedArray from @fluid-experimental/react which depended on SharedObjectSequence.

Changes to examples:

likes-and-comments: Removed the comments portion which relied on SharedObjectSequence, leaving it as just "likes"
badge: Removed history functionality
shared-text: Removed test object
monaco: Removed extraneous registry entry
todo: Convert back to map-based approach
I recognize that some of these example changes are not ideal, but I think we should continue discussion of the scenario in #8526.
@anthony-murphy
Copy link
Contributor Author

this issue was completed via #8585

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

No branches or pull requests

3 participants