[jitsi-dev] duplicate code problems


#1

I found that no matter what I did, jitsi-defaults.properties would not load

I tried patching

   src/net/java/sip/communicator/impl/configuration/JdbcConfigService.java

in the Jitsi repository to not use getSystemResourceAsStream() in web
start. It didn't help. I then tried adding more logging - but I
couldn't see the new log messages.

I tried clearing my Java cache, manually copying the JAR into the cache
on the client, clearing the jitsi settings directory, checking the web
server logs (to see if it was really fetching my new JAR).

However, it turns out I was not working on the right code - the code is
duplicated in the libjitsi project:

   src/org/jitsi/impl/configuration/ConfigurationServiceImpl.java

Is this something very specific to the configuration code or is this a
more common problem in the Jitsi code base?

Is it necessary for a patch to be submitted against both of these source
files?


#2

На 24.07.2014 в 13:54, Daniel Pocock написа:

    src/net/java/sip/communicator/impl/configuration/JdbcConfigService.java
    src/org/jitsi/impl/configuration/ConfigurationServiceImpl.java

LibJitsi's ConfigurationServiceImpl is the default ConfigurationService implementation in general and of Jitsi in particular.

JdbcConfigService is not the default ConfigurationService implementation of Jitsi, it need to be explicitly enabled.

the code is duplicated in the libjitsi project:

JdbcConfigService has duplicated ConfigurationServiceImpl.

Is it necessary for a patch to be submitted against both of these source
files?

Ideally, a patch would eliminate the duplicate source code between ConfigurationServiceImpl and JdbcConfigService and another patch would fix the issue that is of primary interest to you. Anyway, you would be submitting the patch so feel free to submit it against whatever you need patched.


#3

На 24.07.2014 в 13:54, Daniel Pocock написа:

   
src/net/java/sip/communicator/impl/configuration/JdbcConfigService.java
    src/org/jitsi/impl/configuration/ConfigurationServiceImpl.java

LibJitsi's ConfigurationServiceImpl is the default
ConfigurationService implementation in general and of Jitsi in
particular.

JdbcConfigService is not the default ConfigurationService
implementation of Jitsi, it need to be explicitly enabled.

Ok, thanks for clarifying that

I did a string search for "jitsi-defaults.properties" in the Jitsi
repository and JdbcConfigService is the only class that contains the
string, so that is how I started working in there.

the code is duplicated in the libjitsi project:

JdbcConfigService has duplicated ConfigurationServiceImpl.

Is it necessary for a patch to be submitted against both of these source
files?

Ideally, a patch would eliminate the duplicate source code between
ConfigurationServiceImpl and JdbcConfigService and another patch would
fix the issue that is of primary interest to you. Anyway, you would be
submitting the patch so feel free to submit it against whatever you
need patched.

So far I just submitted a patch against libjitsi ConfigurationServiceImpl

Should JdbcConfigService (and potentially other implementations?) be
written as subclasses of ConfigurationServiceImpl perhaps? This might
take a little refactoring.

···

On 24/07/14 13:07, Lyubomir Marinov wrote:


#4

Actually, there's something about this where point of views differ fundamentally...

libjitsi is a library that needs some configuration. However, this config is dependent on the application it runs in (e.g. Jitsi, the Videobridge, etc.).

So, my point of view is that every application should provide its own implementation of the ConfigurationService interface and libjitsi should not even provide a default implementation - it cannot deal with application specific details (like /etc/jitsi or /etc/jitsi-videobridge/...).
So far, the other developers disagreed about that.

And now you come with code duplication :slight_smile:
I wonder how we could avoid that. The two implemenations differ fundamentally and e.g. the videobridge as a server-application for sure doesn't need all that override, transaction and user writable capabilities. All it needs is a default and a config that overrides this from /etc/something.

As for JdbcConfigService: that is intended to replace the libjitsi implementation for Jitsi because reading and writing .properties files is too slow and unreliable for big configs (like Emils 10mb file or my 1mb). It's just not the default (yet). Although, I think we could do that by now, at least for the nightlies. I used it over months without any problems - and AFAIK Emil as well.

Freundliche Grüsse,
Ingo Bauersachs

-- sent from my mobile

···

Le 24.07.2014 à 18:56, "Daniel Pocock" <daniel@pocock.pro> a écrit :

On 24/07/14 13:07, Lyubomir Marinov wrote:
На 24.07.2014 в 13:54, Daniel Pocock написа:

src/net/java/sip/communicator/impl/configuration/JdbcConfigService.java
   src/org/jitsi/impl/configuration/ConfigurationServiceImpl.java

LibJitsi's ConfigurationServiceImpl is the default
ConfigurationService implementation in general and of Jitsi in
particular.

JdbcConfigService is not the default ConfigurationService
implementation of Jitsi, it need to be explicitly enabled.

Ok, thanks for clarifying that

I did a string search for "jitsi-defaults.properties" in the Jitsi
repository and JdbcConfigService is the only class that contains the
string, so that is how I started working in there.

the code is duplicated in the libjitsi project:

JdbcConfigService has duplicated ConfigurationServiceImpl.

Is it necessary for a patch to be submitted against both of these source
files?

Ideally, a patch would eliminate the duplicate source code between
ConfigurationServiceImpl and JdbcConfigService and another patch would
fix the issue that is of primary interest to you. Anyway, you would be
submitting the patch so feel free to submit it against whatever you
need patched.

So far I just submitted a patch against libjitsi ConfigurationServiceImpl

Should JdbcConfigService (and potentially other implementations?) be
written as subclasses of ConfigurationServiceImpl perhaps? This might
take a little refactoring.

_______________________________________________
dev mailing list
dev@jitsi.org
Unsubscribe instructions and other list options:
http://lists.jitsi.org/mailman/listinfo/dev


#5

Actually, there's something about this where point of views differ fundamentally...

libjitsi is a library that needs some configuration. However, this config is dependent on the application it runs in (e.g. Jitsi, the Videobridge, etc.).

So, my point of view is that every application should provide its own implementation of the ConfigurationService interface and libjitsi should not even provide a default implementation - it cannot deal with application specific details (like /etc/jitsi or /etc/jitsi-videobridge/...).
So far, the other developers disagreed about that.

Not exactly. libjitsi is a library and should allow everyone to have
their own coniguration service implementation (which it does) but it
should also have a default one (or even several) that address most
popular use cases. I believe this more accurately describes the second
viewpoint.

And now you come with code duplication :slight_smile:
I wonder how we could avoid that. The two implemenations differ fundamentally and e.g. the videobridge as a server-application for sure doesn't need all that override, transaction and user writable capabilities. All it needs is a default and a config that overrides this from /etc/something.

As for JdbcConfigService: that is intended to replace the libjitsi implementation for Jitsi because reading and writing .properties files is too slow and unreliable for big configs (like Emils 10mb file or my 1mb). It's just not the default (yet). Although, I think we could do that by now, at least for the nightlies. I used it over months without any problems - and AFAIK Emil as well.

Yes I did indeed and it solved a number of performance issues for me,
but then I started having some pretty weird behaviour (which may or
may not have been caused by it) so I started with a new config and
haven't had the time to switch back since.

Emil

···

On Thu, Jul 24, 2014 at 8:41 AM, Ingo Bauersachs <ingo@jitsi.org> wrote:

Freundliche Grüsse,
Ingo Bauersachs

-- sent from my mobile

Le 24.07.2014 à 18:56, "Daniel Pocock" <daniel@pocock.pro> a écrit :

On 24/07/14 13:07, Lyubomir Marinov wrote:
На 24.07.2014 в 13:54, Daniel Pocock написа:

src/net/java/sip/communicator/impl/configuration/JdbcConfigService.java
   src/org/jitsi/impl/configuration/ConfigurationServiceImpl.java

LibJitsi's ConfigurationServiceImpl is the default
ConfigurationService implementation in general and of Jitsi in
particular.

JdbcConfigService is not the default ConfigurationService
implementation of Jitsi, it need to be explicitly enabled.

Ok, thanks for clarifying that

I did a string search for "jitsi-defaults.properties" in the Jitsi
repository and JdbcConfigService is the only class that contains the
string, so that is how I started working in there.

the code is duplicated in the libjitsi project:

JdbcConfigService has duplicated ConfigurationServiceImpl.

Is it necessary for a patch to be submitted against both of these source
files?

Ideally, a patch would eliminate the duplicate source code between
ConfigurationServiceImpl and JdbcConfigService and another patch would
fix the issue that is of primary interest to you. Anyway, you would be
submitting the patch so feel free to submit it against whatever you
need patched.

So far I just submitted a patch against libjitsi ConfigurationServiceImpl

Should JdbcConfigService (and potentially other implementations?) be
written as subclasses of ConfigurationServiceImpl perhaps? This might
take a little refactoring.

_______________________________________________
dev mailing list
dev@jitsi.org
Unsubscribe instructions and other list options:
http://lists.jitsi.org/mailman/listinfo/dev

_______________________________________________
dev mailing list
dev@jitsi.org
Unsubscribe instructions and other list options:
http://lists.jitsi.org/mailman/listinfo/dev

--
https://jitsi.org


#6

I'm not too concerned about where the different interfaces and classes
live, what concerns me more is code duplication. It leads to confusion,
bloats the application and in some cases creates more work to extend things.

Consider a constant definition like this:

private static final String DEFAULT_PROPS_FILE_NAME
        = "jitsi-defaults.properties";

It is repeated in two places. That is really unusual for a constant and
not very intuitive for developers unfamiliar with the code. As a start,
maybe the definition should only be in libjitsi (maybe a dedicated
static class for constants) and then it can be referenced in the
different implementations.

Going a step further, maybe an abstract base class can provide some of
the utility functions, like the new method I contributed in my pull request:

https://github.com/jitsi/libjitsi/pull/13

and the methods for loading properties from an InputStream.

···

On 24/07/14 16:42, Emil Ivov wrote:

On Thu, Jul 24, 2014 at 8:41 AM, Ingo Bauersachs <ingo@jitsi.org> wrote:

Actually, there's something about this where point of views differ fundamentally...

libjitsi is a library that needs some configuration. However, this config is dependent on the application it runs in (e.g. Jitsi, the Videobridge, etc.).

So, my point of view is that every application should provide its own implementation of the ConfigurationService interface and libjitsi should not even provide a default implementation - it cannot deal with application specific details (like /etc/jitsi or /etc/jitsi-videobridge/...).
So far, the other developers disagreed about that.

Not exactly. libjitsi is a library and should allow everyone to have
their own coniguration service implementation (which it does) but it
should also have a default one (or even several) that address most
popular use cases. I believe this more accurately describes the second
viewpoint.


#7

Actually, there's something about this where point of views differ
fundamentally...

libjitsi is a library that needs some configuration. However, this config
is dependent on the application it runs in (e.g. Jitsi, the Videobridge,
etc.).

So, my point of view is that every application should provide its own
implementation of the ConfigurationService interface and libjitsi should not
even provide a default implementation - it cannot deal with application
specific details (like /etc/jitsi or /etc/jitsi-videobridge/...).
So far, the other developers disagreed about that.

Not exactly. libjitsi is a library and should allow everyone to have
their own coniguration service implementation (which it does) but it
should also have a default one (or even several) that address most
popular use cases. I believe this more accurately describes the second
viewpoint.

libjitsi itself wouldn't need anything more than being able to accept a java.util.Properties container. Anything else in the ConfigService are leftovers from Jitsi. The only reason we need to supply (a) default implementation(s) for the ConfigService is because the interface is overblown (for a library) and too hard to implement.

[...]
As for JdbcConfigService: that is intended to replace the libjitsi
implementation for Jitsi because reading and writing .properties files is too
slow and unreliable for big configs (like Emils 10mb file or my 1mb). It's
just not the default (yet). Although, I think we could do that by now, at
least for the nightlies. I used it over months without any problems - and
AFAIK Emil as well.

Yes I did indeed and it solved a number of performance issues for me,
but then I started having some pretty weird behaviour (which may or
may not have been caused by it) so I started with a new config and
haven't had the time to switch back since.

Well, would have been nice to know about that to fix any eventual problems...

Emil

Ingo

···

On 2014-07-24 21:42, Emil Ivov wrote:

On Thu, Jul 24, 2014 at 8:41 AM, Ingo Bauersachs <ingo@jitsi.org> wrote:


#8

I'm not too concerned about where the different interfaces and classes
live, what concerns me more is code duplication. It leads to confusion,
bloats the application and in some cases creates more work to extend

things.

Consider a constant definition like this:

private static final String DEFAULT_PROPS_FILE_NAME
        = "jitsi-defaults.properties";
It is repeated in two places. That is really unusual for a constant and
not very intuitive for developers unfamiliar with the code. As a start,
maybe the definition should only be in libjitsi (maybe a dedicated
static class for constants) and then it can be referenced in the
different implementations.

That is all true, however I chose to duplicate it because I hoped to
dramatically cut down the implementation that lives in libjitsi once the
JdbcConfigService goes into production. And then there would be no more
duplication of constants.

Going a step further, maybe an abstract base class can provide some of
the utility functions, like the new method I contributed in my pull

request:

https://github.com/jitsi/libjitsi/pull/13

I think this can be merged, but it exactly proves my point: why should a
library care that it's loaded from WebStart (or Android for the matter) and
needs to load application specific (not even library specific properties)
differently?

and the methods for loading properties from an InputStream.

Are you referring to actual properties or the defaults?

Ingo

···

On 2014-07-24 23:04, Daniel Pocock wrote: