Added paged catchup message to the protocol #1822
Conversation
Pull reviewers statsStats of the last 30 days for popstellar:
|
…k-fe1-omajzoub-chirps-paging-protocol
There was a problem hiding this comment.
Good starting point :)
Though I would try to clarify the documentation, I doubt that right now somebody unfamiliar with the application would understand.
Also don't forget about testing the schema ;) (https://github.com/dedis/popstellar/blob/master/protocol/test/main.test.js)
docs/protocol.md
Outdated
| By executing a paged catchup action, a client can ask the server to receive a specified number of | ||
| past messages on a specific channel. | ||
|
|
||
| For now, it is to be used to retrieve chirps on the social media channel by paging when a new client joins the LAO instead of getting all chirps at once. This is done in an effort to reduce network traffic at catchup. | ||
|
|
||
| This may serve as a starting point for the paging of messages in other channels as a future optimization. |
There was a problem hiding this comment.
I would try to clarify this further:
What does past messages mean here? (i.e. what is the order)
Why was the message designed this way? (basically what we discussed, this gives some background information)
What is "the social media channel"? I would try to be more precise here.
Also the docs say it is only used for chirps, what should happen if this is sent on some other channel? Is it a no-op for now? Or should the server try to find chirps on any channel and return them?
And which chirp message(-s) should this return? (I understand but the documentation should be precise in this regard)
There was a problem hiding this comment.
Noted
Thanks for the feedback
As for the "Also the docs say it is only used for chirps, what should happen if this is sent on some other channel? Is it a no-op for now? Or should the server try to find chirps on any channel and return them?" question, we decided to not implement paging for other channels this semester.
There was a problem hiding this comment.
I assumed so but the message may be sent by (malicious) actors anyway. The docs should describe how to process the message even if it is sent in an unexpected way. Especially since it should be general in the future!
There was a problem hiding this comment.
I think @Tyratox here makes a very important point.
we decided to not implement paging for other channels this semester.
That's perfectly okay, but what is the expected behavior when it does get called on a non-chirp channel ? Not leaving this kind of ambiguity in the specification is good for two reasons.
- Unexpected stuff happens. Even if there are no malicious actors, a front-end could have bugs and make the wrong RPC call. What happens then ?
- Without a clear specification, I can guarantee that different backends will spontaneously implement this (slightly) differently. It doesn't cost much to clarify and say "we return an error" or "we default to the normal catchup behavior".
- Conversely, if there's a bug and the points above are not addressed, you'll have to debug behaviors like "in some cases, with fe2 and be1 [for instance], the loading of chirps fails with some weird error".
As such:
- if you pick to revert to a reasonable "default" behavior, such as behaving more or less like a catchup (without paging), document that !
- if you pick to return an error, define the error (what code ? what message ?), try and make it super unambiguous and consistent !
Best of luck :-)
There was a problem hiding this comment.
Noted
Thanks for the feedback
...ol/examples/query/paged_catchup/wrong_paged_catchup_missing_everything_after_message_id.json
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Looks overall good to me! It sure is a tricky PR to merge, since it represents a big change that a lot of future work will depend on. Although, I might miss some things, but I still think that the current state looks very usable and accurate to what we want to have.
Very keen to these changes! These will finally relieve front ends from having do compute everything, everytime, on their ow (and needing to query the WHOLE data to do so). I am talking about the fact that the chirps sorting, top chirps calculations and else will now be computed server side 👍 . This is sure a great step towards inclusion of low quality devices/bad connection areas of the world!
docs/protocol.md
Outdated
| to have a separate paging subchannel for each user to avoid sending irrelevant messages to other users. This paging is | ||
| done in an effort to reduce network traffic at catchup. | ||
|
|
||
| This message is also to be used to retrieve chirps of a specific user profile from a subchannel "/root/lao_id/social/profile/user_public_key/specific_user_public_key" where "user_public_key" is the same as before and "specific_user_public_key" is the public key of the user whose messages the client wants to retrieve. Paging is not deemed necessary for retrieving top chirps for now and can be done with the regular catchup message from a subchannel "/root/lao_id/social/top_chirps". |
There was a problem hiding this comment.
I agree that paging for top chirps is not necessary, because for now top chirps are only the 3 first. But we can imagine in the future to be able to scroll down in the same paging way as in the feed, but in top chirps.
What is the top chirps channel supposed to do? Should it be specified? IMO it should send the top 3 on catchup, then send the new top 3 whenever an update is received server side (then front ends should listen to this channel).
Also just a quick note for front ends implementation: I think it would be easier for now to have separate caches for feed/top chirps/profile. Later on, some optimizations could be done (front end wise) to link the caches and avoid duplicate data (chirps) throughout those caches, but it should do without for now as there shouldn't be so much data to handle. Also, I think that we should not cache chirps of profiles (other than ours) since we will very rarely click on those profile (and even less come back to it right after).
There was a problem hiding this comment.
"IMO it should send the top 3 on catchup, then send the new top 3 whenever an update is received server side (then front ends should listen to this channel)." -> Yes that's the plan
"have separate caches for feed/top chirps/profile" -> Makes sense
"we should not cache chirps of profiles (other than ours) since we will very rarely click on those profile" -> I agree
There was a problem hiding this comment.
Are you sure you want to use "/root/lao_id/social/profile/user_public_key/specific_user_public_key"? The location of the user_public_key seems arbitrary.. In the other case it is the last segement, i.e. I want channel x but only for myself, hence x/{myKey}.
Moreover I would try to make it explicit what in a channel string is a parameter and what is a string literal, e.g. /root/lao_id/social/profile/{user_public_key}/{specific_user_public_key}.
Finally, the naming of user_public_key and specific_user_public_key is not ideal.. Maybe use sth like sender / subscriber vs target / profile_user / ... ? Maybe you can come up with sth even better :)
There was a problem hiding this comment.
"IMO it should send the top 3 on catchup, then send the new top 3 whenever an update is received server side (then front ends should listen to this channel)." -> Yes that's the plan
The documentation says
Paging is not deemed necessary for retrieving top chirps for now and can be done with the regular catchup message from a subchannel "/root/lao_id/social/top_chirps".
Why use a regular catchup? It does not allow you to specify the number of top chirps? If this is intentional (e.g. this way the server knows it only has to store the top 3) that works but then you definitely have to update the documentation of the regular catchup message as it will behave differently for one particular channel.
…RL as mentioned in the paged catchup spec in protocol.md
There was a problem hiding this comment.
Good work, the documentation and examples are improving a lot! :)
I added final set of comments. However I don't want to be blocking you from merging this indefinitely and therefore approve it.
However, I would still recommend to go through the comments and at least document the different behavior of the regular catchup message.
docs/protocol.md
Outdated
| to have a separate paging subchannel for each user to avoid sending irrelevant messages to other users. This paging is | ||
| done in an effort to reduce network traffic at catchup. | ||
|
|
||
| This message is also to be used to retrieve chirps of a specific user profile from a subchannel "/root/lao_id/social/profile/user_public_key/specific_user_public_key" where "user_public_key" is the same as before and "specific_user_public_key" is the public key of the user whose messages the client wants to retrieve. Paging is not deemed necessary for retrieving top chirps for now and can be done with the regular catchup message from a subchannel "/root/lao_id/social/top_chirps". |
There was a problem hiding this comment.
Are you sure you want to use "/root/lao_id/social/profile/user_public_key/specific_user_public_key"? The location of the user_public_key seems arbitrary.. In the other case it is the last segement, i.e. I want channel x but only for myself, hence x/{myKey}.
Moreover I would try to make it explicit what in a channel string is a parameter and what is a string literal, e.g. /root/lao_id/social/profile/{user_public_key}/{specific_user_public_key}.
Finally, the naming of user_public_key and specific_user_public_key is not ideal.. Maybe use sth like sender / subscriber vs target / profile_user / ... ? Maybe you can come up with sth even better :)
docs/protocol.md
Outdated
| to have a separate paging subchannel for each user to avoid sending irrelevant messages to other users. This paging is | ||
| done in an effort to reduce network traffic at catchup. | ||
|
|
||
| This message is also to be used to retrieve chirps of a specific user profile from a subchannel "/root/lao_id/social/profile/user_public_key/specific_user_public_key" where "user_public_key" is the same as before and "specific_user_public_key" is the public key of the user whose messages the client wants to retrieve. Paging is not deemed necessary for retrieving top chirps for now and can be done with the regular catchup message from a subchannel "/root/lao_id/social/top_chirps". |
There was a problem hiding this comment.
"IMO it should send the top 3 on catchup, then send the new top 3 whenever an update is received server side (then front ends should listen to this channel)." -> Yes that's the plan
The documentation says
Paging is not deemed necessary for retrieving top chirps for now and can be done with the regular catchup message from a subchannel "/root/lao_id/social/top_chirps".
Why use a regular catchup? It does not allow you to specify the number of top chirps? If this is intentional (e.g. this way the server knows it only has to store the top 3) that works but then you definitely have to update the documentation of the regular catchup message as it will behave differently for one particular channel.
Co-authored-by: Nico Hauser <[email protected]>
Co-authored-by: Nico Hauser <[email protected]>
…thub.com/dedis/popstellar into work-fe1-omajzoub-chirps-paging-protocol
…iteral in protocol.md
…thub.com/dedis/popstellar into work-fe1-omajzoub-chirps-paging-protocol
|
|
|
|
|
In an effort to reduce network traffic at catchup, a paged catchup message was added to the protocol with implementation planned to take place afterwards. For now, it is to be used to retrieve chirps on the social media channel by paging when a new client joins the LAO instead of getting all the chirps at once.