search

apache / openmeetings

Mirror of Apache Openmeetings
Other
639 stars 261 forks source link

OPENMEETINGS-2298 add kstream to connections panel #67

Closed sebawagner closed 4 years ago

sebawagner commented 4 years ago

Add KStreams to Admin>Connections:

I need to look into a proper Unit test for the Wicket ConnectionsPanel.java class. For my new methods. But using WicketTester is not trivial.

Kicking the KStream off is not implemented yet I'm thinking of adding a new ticket for that. As it might be quite involved depending on what that stream currently does.

As you can see, we reference the KStream in 2 places.

So I had to combine those lists. Both can get out of sync. And in fact I think they do: Using above UI you can see that KurentoHandler::rooms::streams doesn't clear. Even closing the browser doesn't clear the KStream.getRoom().getParticipants() Those KStreams stays and in the Admin panel you can still see the KStream Objects. Even though browser with the MediaStream is closed since +10minutes. => Would be worth adding a ticket for it. Separated from this one. But seems the AdminUI is useful.

There is prob more information that can be added to the panel.

solomax commented 4 years ago

How this view should look like?

sebawagner commented 4 years ago

How this view should look like?

What do you mean ? The idea is that the KStreams are listed same as the clients, in the same lists. Two lists is too much effort.

Look like this image

solomax commented 4 years ago

Hello @sebawagner ,

This panel seems to be unique: it displays non DB objects It seems to be worth to refactor things a bit I don't like this useless wrapper-dtos, let native objects be displayed instead :)

sebawagner commented 4 years ago

Yeah I don't mind.

I think separating UI view from DB view has also some long term advantages.

But just for the sake of this 1 screen. I don't want to spend too much time on it. You can potentially make the KStream Serialisable. But this object holds all the MediaStream at cetera. I'm not sure how good that would be to try to serialise it.

I am more keen to get this UI and PR merged, and maybe optimise more later. Cause for me its just a tool to admin and diagnose. And I'm more keen on the diagnosing part.

The other part to keep in mind in that we may enhance this also later more. For instance how about using the kurentoClient to compare KStreams with stream on the server ? To discover any leaks ? Or other statistics that you could extract from the MediaServer that may be useful to diagnose https://doc-kurento.readthedocs.io/en/6.7.1/_static/client-jsdoc/module-kurentoClient.KurentoClient.html#~getServerManagerCallback

Anyway, lets not make this PR too generic. Its just about adding KStream objects to the list and display "some" details in the right hand panel.

solomax commented 4 years ago

Well, I'm not a big fan of separating UI view from DB view I think DTO should be created if it is absolutely necessary

The issues I see here:

  1. non-DB objects are being used as DB objects (by implemeting IDataProviderEntity)
  2. dtos looks useless here

Please merge this PR i'll try to refactor

sebawagner commented 4 years ago

Yeah I understand the IDataProviderEntity doesn't really fit. There is prob also plenty to refactor in that view. But it might be also bit work in progress depending on actions you can do on KStreams.