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

Send msg containing Full Content alongside Diff for database storage #42

Draft
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

gustavotrott
Copy link
Contributor

@gustavotrott gustavotrott commented Jan 16, 2024

Currently, when the text in shared notes is updated, bbb-pads sends a message that contains only the changes made (the diff), rather than the entire updated content. This system relies on both the sending and receiving sides understanding how to generate and apply these diffs. Specifically, bbb-pads generates the diff, and bbb-html5 (Meteor) is responsible for applying this diff to update the content.

However, a new development has arisen: the notes are now going to be stored in a database by akka-apps. The problem is that akka-apps does not possess the functionality to apply these diffs to the existing content.

To resolve this issue, a proposed modifying bbb-pads. Instead of sending only the diff, bbb-pads will now send both the diff and the full updated content. With this change, akka-apps will not need to patch the content with the diff; it can simply store the full, updated content directly into the database, just as it is received from bbb-pads.

So this PR will:

  • Change the message PadContentSysMsg including the full html content instead of only the diff.
  • Reduce default update throttle to 5secs (the read only experience is too bad with 15 secs of delay).
  • Include a script run-dev.sh following the pattern of the other BBB services

Required by: bigbluebutton/bigbluebutton#19437

@@ -11,7 +11,7 @@
"ttl": 21600000
},
"update": {
"throttle": 15000
"throttle": 5000
Copy link
Member

Choose a reason for hiding this comment

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

I suggest using 1 second trottle and see how it goes.

This would provide a better user experience.

Copy link
Member

Choose a reason for hiding this comment

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

@prlanzarin - what do you think?

@pedrobmarin
Copy link
Collaborator

I'm not so sure about it but I think there's already something done in akka-apps that is able to rebuild this text content. Check the captions.
If it doesn't, it's still a diff... what's so absurd about it to be considered useless to akka-apps? I'm sure it would be patchable there.
Pushing full text per message is not a good solution.

@TiagoJacobs
Copy link
Member

Hello @pedrobmarin - we're considering it will be faster (and simpler) to just just passthru the data, and in future we could drop this diff process.

Can you elaborate on the reasoning why this diff exists?

I mean, as we're sending data locally, the throughput is not an issue (while CPU usage is).

@pedrobmarin
Copy link
Collaborator

The diff propagates changes all the way to the users (or at least it used to if nothing changed)

@gustavotrott
Copy link
Contributor Author

The diff propagates changes all the way to the users (or at least it used to if nothing changed)

I believe the diff made sense at that moment.

Now we will provide data through a Graphql API and this data can be consume by Html5-client, Mobile-app, Plugins, 3rth-party applications..
We don't want to make all the clients implement a complex logic to get all the diffs to be able to create the Full content.
The idea is to make it is as simple as possible.

Akka-apps could recreate the full content, but as pointed by @TiagoJacobs, there is no reason to add complexity when it's really simpler to send the full data directly (and remove the diff in the future).

@pedrobmarin
Copy link
Collaborator

Here's a suggestion: create a new event for experimenting this.
Set a new throttle configuration for it. Make sure this won't be propagated to the users. Make all the performance tests you need.
And don't push the dev script in the same commit. If that script is needed. Submit a new pull request with it.

@pedrobmarin
Copy link
Collaborator

Now we will provide data through a Graphql API and this data can be consume by Html5-client, Mobile-app, Plugins, 3rth-party applications..

And how is that going to work? The graphql will push all the text content every time an user needs an update?

@TiagoJacobs
Copy link
Member

Hello @pedrobmarin - thanks for your answer.

And how is that going to work? The graphql will push all the text content every time an user needs an update?

In GraphQl we will have two ways of consuming this data:

  • shared_notes (you can fetch the current full text from here)
  • shared_notes_rev (you can fetch all the revisions from here)

It will be a client decision to consume the full text, or the etherpad revisions.

@pedrobmarin
Copy link
Collaborator

And where are the Etherpad revisions comming from? Are you sure you are using Etherpads revisions?

@TiagoJacobs
Copy link
Member

TiagoJacobs commented Jan 17, 2024

this comment was not precise, removing it

@TiagoJacobs
Copy link
Member

Hello @pedrobmarin - disconsider the last message - @gustavotrott will mention the right callback.

@gustavotrott
Copy link
Contributor Author

Actually the changeset is sent here:

sender.send('padUpdated', meetingId, { groupId, padId, userId, rev, changeset });

@TiagoJacobs
Copy link
Member

This text:
image

Generated these revs:

[
      {
        "rev": 1,
        "changeset": "Z:2>1=1*0+1$O"
      },
      {
        "rev": 2,
        "changeset": "Z:3>1=2*0+1$l"
      },
      {
        "rev": 3,
        "changeset": "Z:4>1=3*0+1$á"
      },
      {
        "rev": 4,
        "changeset": "Z:5>3=4*0|3+3$\n\n\n"
      },
      {
        "rev": 5,
        "changeset": "Z:8>1|3=7*0+1$T"
      },
      {
        "rev": 6,
        "changeset": "Z:9>3|3=7=1*0+3$est"
      },
      {
        "rev": 7,
        "changeset": "Z:c>1|3=7=4*0+1$e"
      },
      {
        "rev": 8,
        "changeset": "Z:d>2|3=7=5*0|2+2$\n\n"
      },
      {
        "rev": 9,
        "changeset": "Z:f>1|5=e*0+1$T"
      },
      {
        "rev": 10,
        "changeset": "Z:g>3|5=e=1*0+3$udo"
      },
      {
        "rev": 11,
        "changeset": "Z:j>4|5=e=4*0+4$ bem"
      },
      {
        "rev": 12,
        "changeset": "Z:n>1|5=e=8*0+1$?"
      },
      {
        "rev": 13,
        "changeset": "Z:o>2|5=e=9*0|2+2$\n\n"
      },
      {
        "rev": 14,
        "changeset": "Z:q>1|7=p*0+1$:"
      },
      {
        "rev": 15,
        "changeset": "Z:r>1|7=p=1*0+1$D"
      },
      {
        "rev": 16,
        "changeset": "Z:s<7|2=6|2-7$"
      }
    ]

@pedrobmarin
Copy link
Collaborator

Ok but I'm still confused about this. Is that pad editable?
The whole point to this diff sharing was to be able to have the locked user view only format without exposing Etherpads vulnerabilities

@pedrobmarin
Copy link
Collaborator

I'm being a little prick about this (and sorry about that) because those things exist for a reason. It took me some time to plan how to make things work for the pads and yet make sure a locked user could not violate they permission.

@TiagoJacobs
Copy link
Member

Hello @pedrobmarin - your feedback is valuable

In order to see our strategy, please try the following:

1 - open the address https://demo-ios.bigbluebutton.org/b/
2 - paste the following script in your console:

const changeSetJson = [
    {
      "rev": 1,
      "changeset": "Z:2>1=1*0+1$H"
    },
    {
      "rev": 2,
      "changeset": "Z:3>2=2*0+2$ey"
    },
    {
      "rev": 3,
      "changeset": "Z:5>1=4*0|1+1$\n"
    },
    {
      "rev": 4,
      "changeset": "Z:6>1|1=5*0|1+1$\n"
    },
    {
      "rev": 5,
      "changeset": "Z:7>1|2=6*0+1$T"
    },
    {
      "rev": 6,
      "changeset": "Z:8>4|2=6=1*0+4$his "
    },
    {
      "rev": 7,
      "changeset": "Z:c>2|2=6=5*0+2$a "
    },
    {
      "rev": 8,
      "changeset": "Z:e<2|2=6=5-2$"
    },
    {
      "rev": 9,
      "changeset": "Z:c>4|2=6=5*0+4$is a"
    },
    {
      "rev": 10,
      "changeset": "Z:g>1|2=6=9*0+1$ "
    },
    {
      "rev": 11,
      "changeset": "Z:h>2|2=6=a*0+2$te"
    },
    {
      "rev": 12,
      "changeset": "Z:j>2|2=6=c*0+2$st"
    },
    {
      "rev": 13,
      "changeset": "Z:l>1|2=6=e*0|1+1$\n"
    },
    {
      "rev": 14,
      "changeset": "Z:m>2|3=l*0|1+1*0+1$\nl"
    },
    {
      "rev": 15,
      "changeset": "Z:o>3|4=m=1*0+3$et'"
    },
    {
      "rev": 16,
      "changeset": "Z:r>1|4=m=4*0+1$s"
    },
    {
      "rev": 17,
      "changeset": "Z:s>1|4=m=5*0+1$ "
    },
    {
      "rev": 18,
      "changeset": "Z:t>2|4=m=6*0+2$se"
    },
    {
      "rev": 19,
      "changeset": "Z:v>4|4=m=8*0+4$e if"
    },
    {
      "rev": 20,
      "changeset": "Z:z>1|4=m=c*0+1$ "
    },
    {
      "rev": 21,
      "changeset": "Z:10>3|4=m=d*0+3$we "
    },
    {
      "rev": 22,
      "changeset": "Z:13>4|4=m=g*0+4$can "
    },
    {
      "rev": 23,
      "changeset": "Z:17>1|4=m=k*0+1$r"
    },
    {
      "rev": 24,
      "changeset": "Z:18>1|4=m=l*0+1$e"
    },
    {
      "rev": 25,
      "changeset": "Z:19>1|4=m=m*0+1$b"
    },
    {
      "rev": 26,
      "changeset": "Z:1a>3|4=m=n*0+3$uil"
    },
    {
      "rev": 27,
      "changeset": "Z:1d>3|4=m=q*0+3$d t"
    },
    {
      "rev": 28,
      "changeset": "Z:1g>1|4=m=t*0+1$h"
    },
    {
      "rev": 29,
      "changeset": "Z:1h>3|4=m=u*0+3$is "
    },
    {
      "rev": 30,
      "changeset": "Z:1k>1|4=m=x*0+1$t"
    },
    {
      "rev": 31,
      "changeset": "Z:1l>2|4=m=y*0+2$es"
    },
    {
      "rev": 32,
      "changeset": "Z:1n>2|4=m=z-1*0+3$xt "
    },
    {
      "rev": 33,
      "changeset": "Z:1p>1|4=m=12*0+1$u"
    },
    {
      "rev": 34,
      "changeset": "Z:1q>5|4=m=13*0+5$sing "
    },
    {
      "rev": 35,
      "changeset": "Z:1v>2|4=m=18*0+2$an"
    },
    {
      "rev": 36,
      "changeset": "Z:1x>5|4=m=1a*0+5$other"
    },
    {
      "rev": 37,
      "changeset": "Z:22>5|4=m=1f*0+5$ clie"
    },
    {
      "rev": 38,
      "changeset": "Z:27>2|4=m=1k*0+2$nt"
    },
    {
      "rev": 39,
      "changeset": "Z:29<1n|3=l|1-1-1m$"
    },
    {
      "rev": 40,
      "changeset": "Z:m>1|3=l*0|1+1$\n"
    },
    {
      "rev": 41,
      "changeset": "Z:n>1|4=m*0|1+1$\n"
    },
    {
      "rev": 42,
      "changeset": "Z:o>2|5=n*0+2$OK"
    },
    {
      "rev": 43,
      "changeset": "Z:q>2|5=n=2*0+2$, "
    },
    {
      "rev": 44,
      "changeset": "Z:s>2|5=n=4*0+2$by"
    },
    {
      "rev": 45,
      "changeset": "Z:u>1|5=n=6*0+1$e"
    }
  ];


function parseIt () {
    const changeSet = require('ep_etherpad-lite/static/js/Changeset.js');
    let texto = "  ";
    changeSetJson.forEach( cs => texto = changeSet.applyToText(cs.changeset, texto) );
    console.log("Texto final: ", texto);
}

function loadScript4() {
    var script4 = document.createElement("script");
    script4.src="https://demo-ios.bigbluebutton.org/pad/javascripts/lib/ep_etherpad-lite/static/js/ace2_common.js?callback=require.define&v=b9249475";
    document.body.appendChild(script4);
    script4.onload = parseIt;
    console.log("loading script4");
}

function loadScript3() {
    var script3 = document.createElement("script");
    script3.src="https://demo-ios.bigbluebutton.org/pad/javascripts/lib/ep_etherpad-lite/static/js/ace2_inner.js?callback=require.define&v=b9249475";
    document.body.appendChild(script3);
    script3.onload=loadScript4;
    console.log("loading script3");
}

function loadScript2() {
    var script2 = document.createElement("script");
    script2.src="https://demo-ios.bigbluebutton.org/pad/javascripts/lib/ep_etherpad-lite/static/js/pad.js?callback=require.define&v=b9249475";
    script2.onload=loadScript3;
    document.body.appendChild(script2);
    console.log("loading script2");
}

var script1 = document.createElement("script");
script1.src="https://demo-ios.bigbluebutton.org/pad/static/js/require-kernel.js?v=b9249475";
script1.onload=loadScript2;
document.body.appendChild(script1);
console.log("loading script1");

You will see the reassembled text:
image

@TiagoJacobs
Copy link
Member

Hello @pedrobmarin - just adding more information, this way the HTML client will render the read-only pads using the revisions, and the "full text" will be sent to akka for adding support to simpler clients (like a plugin that wants to fetch the current content and export to a format).

I agree with you that we can add a different message for publishing the full HTML (and have a different throttle configuration for it).

Also, good point about adding the run-dev script in a separate PR.

@gustavotrott - can you proceed with these changes?

@pedrobmarin
Copy link
Collaborator

Oh, ok. Gotcha. You're using Etherpads scripts to rebuild it.
So yeah, the suggestion I made before stands. Try doing this in a new event and if everything works as expected you can remove the old events after.
Just be sure to check the captions as well because they use part of this implementation.
Another thing you can do is check the pluginwe use to publish etherpads events in Redis. Maybe you can pick everything you need and push it to a new event there. Then maybe you could just listen to that new event in akka-apps.

@gustavotrott gustavotrott marked this pull request as draft January 17, 2024 17:26
@prlanzarin prlanzarin removed their request for review August 23, 2024 17:26
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.

3 participants