Skip to content
This repository was archived by the owner on Apr 13, 2022. It is now read-only.

[proposed spec change] subscribe to whole subtree of container - #191

Closed
michielbdejong wants to merge 2 commits into
masterfrom
michielbdejong-patch-1
Closed

michielbdejong wants to merge 2 commits into
masterfrom
michielbdejong-patch-1

Conversation

@michielbdejong

Copy link
Copy Markdown
Contributor

No description provided.

@michielbdejong

Copy link
Copy Markdown
Contributor Author

no need to review this now, just documenting that this is something we're experimenting with in https://github.com/inrupt/websockets-pubsub

@michielbdejong michielbdejong added the proposed-spec-change May or may not be adopted in future spec versions. Experimental; not yet universally supported. label Jun 18, 2019
@michielbdejong

Copy link
Copy Markdown
Contributor Author

See also #180

@michielbdejong

Copy link
Copy Markdown
Contributor Author

Do not merge unless NSS also implements this.

Comment thread api-websockets.md Resolved
@RubenVerborgh

Copy link
Copy Markdown
Contributor

Thanks for clarifying the recursive part, @michielbdejong.

At this point, I want to express my concern regarding the number of notifications that will nee to be generated by the server, and processed by the receiver.

If we want recursive notifications, we might want to make such recursive subscriptions a) explicitly opt-in (so non-recursive is also still possible) and/or b) optional for the server.

@michielbdejong

michielbdejong commented Jun 18, 2019 •

Copy link
Copy Markdown
Contributor Author

Yes, I see your point. But note that notifications will only be sent for resources that the app has read access to.

And even without that change, the number notifications to be sent (number of resources in a single container that the client is subscribed to) might be prohibitive, so this will generally increases the number in practice, but the worst-case number was already huge.

@RubenVerborgh

Copy link
Copy Markdown
Contributor

But note that notifications will only be sent for resources that the app has read access to.

That doesn't necessarily make it better for the server though, because now every change within a subscribed subfolder should be ran through the ACL process.

the number notifications to be sent (number of resources in a single container that the client is subscribed to) might be prohibitive, so this will generally increases the number in practice, but the worst-case number was already huge.

Yes, but it used to be the case that you needed to do a lot of actions to get a large volume; now a single action can get you a large volume. That works both ways.

This reinforces my belief that we should design an update subscription mechanism instead of extending a quick hack that happened to be built at some point (#179 (comment)).

@RubenVerborgh RubenVerborgh left a comment

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.

Concerns about scalability, and whether we should not design a proper notification mechanism from scratch. In particular, we should examine whether recursive subscriptions should be explicit and/or optional for the server.

@michielbdejong

Copy link
Copy Markdown
Contributor Author

every change within a subscribed subfolder should be ran through the ACL process.

that's already the case right? also if container subscription is not recursive?

design a proper notification mechanism from scratch

Sure, go ahead and propose something! Let me know if you want input. This PR just documents what inrupt/pod-server currently does.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

proposed-spec-change May or may not be adopted in future spec versions. Experimental; not yet universally supported.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants