Okay, then let's try to use StreamProcessor as the place to store the reference to the KStreams. No interface.
And I will put up a PR to refactor that. And you can have a look Cheers Seb Sebastian Wagner https://www.linkedin.com/in/sebastianwagner/ On Tue, 28 Apr 2020 at 18:13, Maxim Solodovnik <solomax...@gmail.com> wrote: > On Tue, 28 Apr 2020 at 03:29, seba.wag...@gmail.com > <seba.wag...@gmail.com> wrote: > > > > I understand the motivation for the 2 lists. But you could even have 2 > lists but in ONE class. > > > > This should be abstracted into one place, that has a defined API to > access the KStream. So that no consumer comes up with the idea to bypass > the methods to directly access a KStream. Other then the defined API > methods of add/remove/getByStreamUid/get (or whatever else we come up with). > > Well it sounds logical to store something belongs to particular room > in this room > Unification might be something good but might be absolute evil > > Let's start with the PR so will have something to discuss > > So far I can see difficulties with having multi-level hash table for > KStreams > Let's see > > > > > The reason for the Interface is _not_ to have multiple implementations. > It never was for the IClientManager either. The idea is that > > - Its 100% clear which methods you can use to get/remove/add a stream. > And you don't come up with other random methods to add/remove "things" > because in the middle of some other method it seems handy to call add/remove > > - You can use those methods to hide any optimisation _behind_ it. For > instance the optimisation with 2 lists: If they would be hidden behind a > common method that adds and removes it in one place: Storing it in two > lists wouldn't be a problem! Cause it's an atomic operation. You won't be > able to remove it in one place without updating the other. > > > > I see no value in interface here > Make methods public, hide others with private > > Interfaces are good to > 1) have several implementations > 2) expose class implemented in different module > > > So yeah I see your point. But having those ghost connections => It is > -worth it- to fix this in a way that its a clear separation of concerns: > > 1) Doing operations ON the stream > > 2) Managing/adding/removing the stream > > > > And not mix this into one class. > > > > This managing of connections and sessions is really at the heart of the > Web-Conferencing application. > > > > You can just see those reports of people claiming "users were still in > the conference room after leaving". That is deeply mistrusting of what we > do. And really hard to reverse engineer the use-case. There are so many! > How many different combinations of entering and leaving a room can you do? > And what if somebody closes the browser in the middle of the room entering? > Its impossible to reverse engineer. And a -maybe even overly careful- > designed separation will be useful. > > All these tickets can only illustrate "having connections in one > place" will not solve the problem :( > All Client objects currently being stored in one place > "users were still in the conference room after leaving" is caused by > some client side JS error (it MUST be deleted on WebSocket connection > lost, but unfortunately it happens) > > > > > I created two tickets: > > https://issues.apache.org/jira/browse/OPENMEETINGS-2315 > > https://issues.apache.org/jira/browse/OPENMEETINGS-2315 > > > > Once we agreed on what we do. > > > > Cheers > > Seb > > > > Sebastian Wagner > > https://www.linkedin.com/in/sebastianwagner/ > > > > > > On Mon, 27 Apr 2020 at 20:46, Maxim Solodovnik <solomax...@gmail.com> > wrote: > >> > >> Hello Sebastian, > >> > >> If I do remember correctly `StreamProcessor::streamByUid <Map with > >> Streams by UID>` was created to speed-up stream search (to avoid > >> iterating all KRooms) > >> The main source-of-truth should be KRoom > >> > >> Duplication is bad thing - no doubt but I thought extra-iterating is > worst :) > >> I tried to make both consistent ... are you sure they are out of sync? > >> > >> I see no value in IKStreamManager due to: > >> 1) it will be implemented only once > >> 2) I expect no inter-module troubles here > >> > >> Maybe we can get rid of KRoom map due to stream UID should be unique > >> and streamByUid can be retrieved in constant time > >> > >> Does it make sense? > >> > >> On Mon, 27 Apr 2020 at 10:25, seba.wag...@gmail.com > >> <seba.wag...@gmail.com> wrote: > >> > > >> > I also can see that those lists have slightly different meaning > >> > > >> > 1. StreamProcessor::streamByUid <Map with Streams by UID> > >> > => Will be populated when the stream start > >> > 2. KRoom::streams <Map with Streams by UID> > >> > => Will be populated when KStream is created > >> > > >> > Still that would be no problem to replicate this functionality in 1 > place. > >> > > >> > Cheers > >> > Seb > >> > > >> > Sebastian Wagner > >> > https://www.linkedin.com/in/sebastianwagner/ > >> > > >> > > >> > On Mon, 27 Apr 2020 at 13:43, seba.wag...@gmail.com < > seba.wag...@gmail.com> wrote: > >> >> > >> >> This should be actually a pretty simple refactor. And much easier > than trying to replicate StreamHandler events in to the KRoom. > >> >> > >> >> As well as having a defined API in the IKStreamHandler. > >> >> > >> >> Cheers > >> >> Seb > >> >> > >> >> > >> >> Sebastian Wagner > >> >> https://www.linkedin.com/in/sebastianwagner/ > >> >> > >> >> > >> >> On Mon, 27 Apr 2020 at 10:33, seba.wag...@gmail.com < > seba.wag...@gmail.com> wrote: > >> >>> > >> >>> Hi, > >> >>> > >> >>> currently we have 2 places that hold a reference to the KStream > objects: > >> >>> > >> >>> StreamProcessor::streamByUid <Map with Streams by UID> > >> >>> KRoom::streams <Map with Streams by UID> > >> >>> > >> >>> Removing it from one place may or may not result removing it in the > other place. > >> >>> This can lead to memory leaks. As well as those lists can get out > of sync. > >> >>> And you are also duplicating code in two places. > >> >>> > >> >>> Could we explore if we can add a similar construct to the KStream > that exists for Client objects using an abstract IClientManager and an > implementation ClientManager ? > >> >>> > >> >>> Similar adding a Class KStreamManager and an Interface > IKStreamManager. And reference this single object in both places > >> >>> IKStreamManager can define methods to get either Streams by RoomId > or by UID or by SID. Or other abstract methods. > >> >>> > >> >>> This could help a lot with making sure when adding or removing a > KStream, there is a single place that holds the reference. > >> >>> > >> >>> What do you think? > >> >>> > >> >>> Thanks, > >> >>> Seb > >> >>> > >> >>> Sebastian Wagner > >> >>> https://www.linkedin.com/in/sebastianwagner/ > >> > >> > >> > >> -- > >> Best regards, > >> Maxim > > > > -- > Best regards, > Maxim >
