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). 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. 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. 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 >
