You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@openmeetings.apache.org by GitBox <gi...@apache.org> on 2021/03/04 07:54:42 UTC
[GitHub] [openmeetings] sebawagner opened a new pull request #135: OPENMEETINGS-2585 IKurentoHandler class and add methods to IStreamHandler
sebawagner opened a new pull request #135:
URL: https://github.com/apache/openmeetings/pull/135
**This can be merged after the 6.0.0 release.**
The IKurentoHandler interface enables configuration of different implementation.
Adding also more methods to IStreamHandler.
It was also possible to refactor some bean references and it may make testing a bit easier now.
Those interfaces also give a good idea how much functionality is in them.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [openmeetings] sebawagner closed pull request #135: PoC - OPENMEETINGS-2585 IKurentoHandler class and add methods to IStreamHandler
Posted by GitBox <gi...@apache.org>.
sebawagner closed pull request #135:
URL: https://github.com/apache/openmeetings/pull/135
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [openmeetings] sebawagner commented on a change in pull request #135: OPENMEETINGS-2585 IKurentoHandler class and add methods to IStreamHandler
Posted by GitBox <gi...@apache.org>.
sebawagner commented on a change in pull request #135:
URL: https://github.com/apache/openmeetings/pull/135#discussion_r587245236
##########
File path: openmeetings-web/src/main/java/org/apache/openmeetings/web/admin/connection/ConnectionsPanel.java
##########
@@ -57,9 +57,9 @@
@SpringBean
private ClientManager cm;
Review comment:
We created the interface IClientManager to make sure we use the right kind of API methods to access the clientManager (and to potentially change the client manager into another storage option, like Redis or sth - something that doesn't require increasing memory with number of users!)
But if we then wire up the concrete class instead of the Interface, that will break it! You can't swap IClientManager with a different implementation anymore.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [openmeetings] sebawagner commented on pull request #135: OPENMEETINGS-2585 IKurentoHandler class and add methods to IStreamHandler
Posted by GitBox <gi...@apache.org>.
sebawagner commented on pull request #135:
URL: https://github.com/apache/openmeetings/pull/135#issuecomment-790439102
> I'm -1 for the changes like it is here :(
>
> we definitely not need these huge interfaces at this stage
> also we don't need to make all methods public as well as some are not static anymore
>
> I would suggest to make `necessary changes` only
You can probably refactor some of the methods further to get the size of the interface smaller.
The problem is: Those classes have never been designed in a way so you can easily introduce a new behaviour. Except all new behaviour is in the OpenMeetings-core module.
There is a lot of functionality in the StreamProcessor and KurentoHandler that have nothing to do with Streaming or Kurento:
- All those "static" methods. Why are they in those classes? They belong into a Util class.
- There are a lot of methods to "checkRights" in the StreamProcessor, what for? Those methods only check on the "Client" nothing to do with Streaming or StreamProcessor
- A lot of the methods are just "facade", for example streamProcessor::isSharing => Calls kHandler.getRoom() => Why not directly calls kHandler.getRoom() ? I has nothing to do with Streaming.
- StreamProcessor::startConvertion => Why is that in the StreamProcessor ? It starts a new thread to process a recording/. It has nothing to do on the stream. Doesn't require anything from it. There is just no reason why its in this class
And the list could be going on for a lot longer.
I think it would be great to refactor those methods out of those classes. And then those interfaces would become a lot smaller and better (and also a lot easier to use!).
And then it would be a lot easier to actually do the different Kurento and Stream handling! Cause you don't need to worry about all those other internal OpenMeetings functionality!
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [openmeetings] solomax commented on a change in pull request #135: OPENMEETINGS-2585 IKurentoHandler class and add methods to IStreamHandler
Posted by GitBox <gi...@apache.org>.
solomax commented on a change in pull request #135:
URL: https://github.com/apache/openmeetings/pull/135#discussion_r587246606
##########
File path: openmeetings-web/src/main/java/org/apache/openmeetings/web/admin/connection/ConnectionsPanel.java
##########
@@ -57,9 +57,9 @@
@SpringBean
private ClientManager cm;
Review comment:
`IClientManager` was created since we split our application to modules
To allow access to some client manager features from outside `openmeetings-web`
this is HACK :(
all these interfaces are useless ...
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [openmeetings] sebawagner commented on a change in pull request #135: OPENMEETINGS-2585 IKurentoHandler class and add methods to IStreamHandler
Posted by GitBox <gi...@apache.org>.
sebawagner commented on a change in pull request #135:
URL: https://github.com/apache/openmeetings/pull/135#discussion_r587237201
##########
File path: openmeetings-web/src/main/java/org/apache/openmeetings/web/admin/connection/ConnectionsPanel.java
##########
@@ -57,9 +57,9 @@
@SpringBean
private ClientManager cm;
Review comment:
This should be probably also IClientManager
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [openmeetings] sebawagner edited a comment on pull request #135: OPENMEETINGS-2585 IKurentoHandler class and add methods to IStreamHandler
Posted by GitBox <gi...@apache.org>.
sebawagner edited a comment on pull request #135:
URL: https://github.com/apache/openmeetings/pull/135#issuecomment-790439102
> I'm -1 for the changes like it is here :(
>
> we definitely not need these huge interfaces at this stage
> also we don't need to make all methods public as well as some are not static anymore
>
> I would suggest to make `necessary changes` only
You can probably refactor (some) sry A LOT of the methods further to get the size of the interface smaller.
The problem is: Those classes have never been designed in a way so you can easily introduce a new behaviour. Except all new behaviour is in the OpenMeetings-core module.
There is a lot of functionality in the StreamProcessor and KurentoHandler that have nothing to do with Streaming or Kurento:
- All those "static" methods. Why are they in those classes? They belong into a Util class.
- There are a lot of methods to "checkRights" in the StreamProcessor, what for? Those methods only check on the "Client" nothing to do with Streaming or StreamProcessor
- A lot of the methods are just "facade", for example streamProcessor::isSharing => Calls kHandler.getRoom() => Why not directly calls kHandler.getRoom() ? I has nothing to do with Streaming.
- StreamProcessor::startConvertion => Why is that in the StreamProcessor ? It starts a new thread to process a recording/. It has nothing to do on the stream. Doesn't require anything from it. There is just no reason why its in this class
And the list could be going on for a lot longer.
I think it would be great to refactor those methods out of those classes. And then those interfaces would become a lot smaller and better (and also a lot easier to use!).
And then it would be a lot easier to actually do the different Kurento and Stream handling! Cause you don't need to worry about all those other internal OpenMeetings functionality!
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [openmeetings] solomax commented on pull request #135: OPENMEETINGS-2585 IKurentoHandler class and add methods to IStreamHandler
Posted by GitBox <gi...@apache.org>.
solomax commented on pull request #135:
URL: https://github.com/apache/openmeetings/pull/135#issuecomment-790410297
I'm -1 for the changes like it is here :(
we definitely not need these huge interfaces at this stage
also we don't need to make all methods public as well as some are not static anymore
I would suggest to make `necessary changes` only
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [openmeetings] solomax commented on pull request #135: OPENMEETINGS-2585 IKurentoHandler class and add methods to IStreamHandler
Posted by GitBox <gi...@apache.org>.
solomax commented on pull request #135:
URL: https://github.com/apache/openmeetings/pull/135#issuecomment-790412237
> I was running into this issue, and I had to do this fix, apparently IApplication can't be found. I think it has to do because this bean gets loaded earlier due to KurentoHandler being explicit bean in applicationContext.
one more reason to find better way to manage this than `applicationContext.xml`
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [openmeetings] sebawagner commented on pull request #135: PoC - OPENMEETINGS-2585 IKurentoHandler class and add methods to IStreamHandler
Posted by GitBox <gi...@apache.org>.
sebawagner commented on pull request #135:
URL: https://github.com/apache/openmeetings/pull/135#issuecomment-791615059
Closed
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [openmeetings] solomax commented on a change in pull request #135: OPENMEETINGS-2585 IKurentoHandler class and add methods to IStreamHandler
Posted by GitBox <gi...@apache.org>.
solomax commented on a change in pull request #135:
URL: https://github.com/apache/openmeetings/pull/135#discussion_r587241756
##########
File path: openmeetings-web/src/main/java/org/apache/openmeetings/web/admin/connection/ConnectionsPanel.java
##########
@@ -57,9 +57,9 @@
@SpringBean
private ClientManager cm;
Review comment:
why?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [openmeetings] sebawagner commented on a change in pull request #135: OPENMEETINGS-2585 IKurentoHandler class and add methods to IStreamHandler
Posted by GitBox <gi...@apache.org>.
sebawagner commented on a change in pull request #135:
URL: https://github.com/apache/openmeetings/pull/135#discussion_r587245236
##########
File path: openmeetings-web/src/main/java/org/apache/openmeetings/web/admin/connection/ConnectionsPanel.java
##########
@@ -57,9 +57,9 @@
@SpringBean
private ClientManager cm;
Review comment:
We created the interface IClientManager to make sure we use the right kind of API methods to access the clientManager (and to potentially change the client manager into another storage option, like Redis or sth - something that doesn't require increasing memory with number of users!)
But if we then wire up the concrete class instead of the Interface, that will break it! You can't swap ClientManager with a different implementation.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [openmeetings] sebawagner commented on a change in pull request #135: OPENMEETINGS-2585 IKurentoHandler class and add methods to IStreamHandler
Posted by GitBox <gi...@apache.org>.
sebawagner commented on a change in pull request #135:
URL: https://github.com/apache/openmeetings/pull/135#discussion_r587237835
##########
File path: openmeetings-web/src/main/java/org/apache/openmeetings/web/admin/connection/ConnectionsPanel.java
##########
@@ -57,9 +57,9 @@
@SpringBean
private ClientManager cm;
Review comment:
`private IClientManager cm;`
Should be probably also IClientManager
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [openmeetings] sebawagner commented on a change in pull request #135: OPENMEETINGS-2585 IKurentoHandler class and add methods to IStreamHandler
Posted by GitBox <gi...@apache.org>.
sebawagner commented on a change in pull request #135:
URL: https://github.com/apache/openmeetings/pull/135#discussion_r587237835
##########
File path: openmeetings-web/src/main/java/org/apache/openmeetings/web/admin/connection/ConnectionsPanel.java
##########
@@ -57,9 +57,9 @@
@SpringBean
private ClientManager cm;
Review comment:
Should be probably also IClientManager
```suggestion
private IClientManager cm;
```
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org