wojciech.kapcia@tigase.net opened 8 years ago
|
|
I would like to add a few comments.
That is partially true. It will be loaded and active if there is no explicit component configuration stating
will never be loaded if it will not be mentioned in configuration.
will be loaded always (as Kernel.class is always a bean extending Kernel.class)
will be loaded only if Due to that, I think this is proper behaviour as you exactly know what to expect and dependent beans are automatically enabled/activated when needed. Also please not that most of beans needs to be annotated with If we would invert this behaviour then it would be a mess as you would need to enable optional component (bean) and all beans related to component bean, ie. repository pool bean, discovery module bean, ping module bean, etc. As an alternative solution (if you decide we need to change this) I would suggest to force developer to pass explicit value to |
|
Andrzej Wójcik wrote:
Comment came from a quick observation that with the really quick observation that with a relatively empty config and more-or less default distribution I ended up with a lot of components being loaded and activated (http-api and licenceserver for example). This looks a bit like a regression/change in desired behaviour in comparison to previous versions where you had to explicitly enable component to be loaded (but this would break quickly with the whole dependency injection).
Would it be possible to differentiate behaviour based on the parent class? For example It should be feasible (and you commented during the call that injection works based on correct levels which would indicate that at some level we have a "proper component bean"), or add explicit property denoting concrete component?
I think this question is more for %kobit - what defaults and default behaviour we want to have and maintain.
I don't think enforcing this is the best idea, but if nothing else works this may be a middle-ground solution. |
|
Wojciech Kapcia wrote:
Exactly, but we are now in a point in which we already did a big change - introduction of dependency injection and Tigase Kernel. Due to that I think it may be possible to change this behavior at this point. %kobit What do you think?
I do not like this idea - we should keep things simple and all beans should behave in a same way. Please keep in mind that it is always possible to mark component beans with @active=false@, so what is point of enforcing it and making it more complicated (even for user)?
I think this is best thing we can do. |
|
Andrzej Wójcik wrote:
The question remains whether new behaviour makes sense (opt-out) - I would say that manually enabling desired components (apart from defaults) should be maintained (in mi mind this feels like buying new laptop with LeadingOperatingSystem which loads every bit of software it came with instead of only what's needed allowing you to add what you need; keep in mind that in |
|
%wojtek I agree with that. That's why most of components (optional components) is annotated with Only HTTP API was marked with Also if you would look back at version 7.1.0, there is a lot of components which are enabled/active by default without any configuration being written, ie. AMP, C2S, BOSH, S2S, etc. and you need to manually turn them off, so we already had an opt-out. |
|
Andrzej Wójcik wrote:
%andrzej.wojcik I'm aware of that - but still it was mostly intended for running a web installer and after running setup it would/should be possible to disable it (was it disable automatically? or the config was kept?). I would say that keeping it that way would be preferred as http-api component doesn't seem essential to XMPP per se.
I know, but those looks like a "core component" and having them enabled by default makes sense (CMs enable communication, SM enable handling user sessions, etc); MUC, PubSub, MA/UA on the other hand can be considered as additional functionality. Please bare in mind, that mentioned defaults depended also on |
|
I am not sure if I follow the discussion correctly. I am (hopefully) looking at it from the end-user perspective, so my expectation (best user experience) is that:
|
|
Artur Hefczyc wrote:
(I skipped the other comments as Tigase works like it currently) That's the clue of the issue - right now (in 7.2.0) Tigase will load all beans that it will find in classpath (@jars/@) regardless of them being enabled in the configuration. It will not load a component if:
So the proposed solution is to enforce setting this in bean configuration in the source (currently MUC and PubSub are made this way: @@Bean(name = "muc", parent = Kernel.class, active = false,…@) |
|
Yes, this is correct behavior in my opinion. Actually, from the end-user point of view, it works exactly the same as the previous Tigase XMPP Server versions. I mean, there was a set of components and plugins which were loaded by default, unless explicitly disabled in the configuration file. So, yes, we can continue this with the new behavior and control default set of components from the source code (as in previous versions) by setting bean active to false or true. However, I think, now, we can load more components, than in the old version by default. It makes sense to load additional components which might not be loaded by default previously:
The only components which should not be loaded by default, which come to my mind are:
|
|
I disagree that we should load by default other new components such as UA, MA, MUC, PubSub as they are not core server components and are not required nor expected to be active (as they weren't in previous version). I've enabled HTTP API as setup depends on this component and features like UI and Admin UI will not work without it and are expected by end user to be available by default. As for other components - they are now very easy to enable, ie. to enable PubSub just add:
and component will be activated. No need to pass class names, etc. |
|
Artur Hefczyc wrote:
Yes, this is true, but there is a tiny change - for the component to be default you would have to specify it as such (lists in Configurator), currently you have to explicitly mark component (or any bean for that matter) as @active=false@. And because not all components are, it results in loading more than desired/being obvious. We can continue with it but I think we should make it mandatory to include I'm not so sure we should enable MUC/PubSub/UA (especially UA, as it's not standard as) - we have a webinstaller which allows selecting desired components. So far list of default components consist of "core" components (message-router, session-manager, connection managers) which IMHO makes sense - it allows basic operation of the server.
This component is not publicly available nor bundled. |
|
As for the default set of components we have to look at it from the end-user perspective. A user who installs an application with default configuration expects all the standard functionality to be included. So, defaults we had years ago and which we had for year do not necessarily are the best right now. Nowadays MUC and PubSub is pretty much standard and expected to be supported out of the box. AMP and MA is less obvious as the support is not so widely available yet. However, UA offers something special, I believe it is suppers solution over MA and we should promote it. One of the way to promote it would is to include it in a default list of components loaded into server. So, as long as the UA is backward compatible with MA (XEP-0136) I would like it to be loaded by default. HTTP API should be enabled as well by default, even after the installation is complete. REST API is pretty much a standard for accessing various services and many people expect it to be available. I agree that the |
|
Artur Hefczyc wrote:
Same here.
There is distinction with the default installer components (which generates resulting configuration file) and default components loaded, example below.
While I agree those are considered default and has been set as such in the installer since the beginning we are switching from following (results in loading MR/SM/all CMs):
to (which would load everything that is not marked as
So with the current behaviour after someone deselects MUC and PubSub it would end-up with following config (and other not correctly annoted now) is that correct? :
I agree that the change is quite minute, but IMHO it feels (felt) a bit random at first.
Again - documentation and clear explanation what this property is and what it does (including javadoc) is a must I think. And appropriately marking all our components as %andrzej.wojcik a question - if the component is marked as |
|
%wojtek Yes, you are correct. |
|
Wojciech Kapcia wrote:
Yes, this is correct. And of course, all the components which are not correctly annotated, can be easily fixed, can they? It does not look like a complex task?
Yes, we need a good documentation on this topic. And in general we need a good documentation on the new kernel stuff and new API. I have to admit it is quite complex for me. |
|
Artur Hefczyc wrote:
It can be fixes (or better yet, as explained above, make this annotation required). %kobit - can you confirm list of default, desired component active by default (stated in #4888#note-10)?
%kobit |
|
Wojciech Kapcia wrote:
Additional components which I would like to be loaded by default:
|
|
Andrzej, can you make changes as per discussion:
|
|
Wojciech Kapcia wrote:
Sure, no problem. I will do that.
I cannot agree to enable UA by default. Can we enable MA by default? Artur, I know that you would prefer to enable UA as it is extension to the MA, but UA supports only databases to store data (JDBC only). So if we would enable UA by default then we would get errors if anyone would decide to use Tigase with MongoDB. While I agree that UA has better features, I think we should active by default only components which supports all data stores supported by Tigase. To deal with this I would like to suggest to enable MA by default and later on, when UA will get support for MongoDB we would enable if by default, do you agree? +Note:+ Adding support for MongoDB to UA is very tricky thing as UA uses a lot of complex queries, which are difficult to implement in data store such as MongoDB. |
|
Ok, that makes sense. Let's start with MA as a default component. |
|
I've enabled:
I decided not to enable PEP by default as it would increase load on a server as each I've not activated AMP as it was already enabled for a long time. However after I did all of that, I have a few additional questions:
In my opinion it is expected to have PEP in latests XMPP server available.
I have no idea what to do in this case. Maybe PubSub and MUC should be disabled in cluster mode by default? but there will be inconsistency with PubSub and MUC being enabled in non-cluster mode. |
|
Andrzej Wójcik wrote:
Yes, I agree, it should be active by default. Again, another point to the performance related information/documentation.
I think majority of "default" installation installed with web installer would be for testing/development purposes in a single (non-clustered) mode. So this must work without any problems. It is not acceptable that the server shuts-down with default configuration with ACS not enabled.
First question which comes to my mind. How is this different from SM? Don't we have the same problem with SM?
|
|
Artur Hefczyc wrote:
Ok, I will need to enable this.
I will skip rest of a comment as I think we have a misunderstanding here. Let me explain how it works now (and I think it is near to ideal):
In second case, if ACS is disabled and MUC or PubSub are enabled then Tigase will decline to start as clustered versions of MUC and PubSub require ACS to work. So the questions here are:
In my opinion (after I reviewed you comment), I think we can leave it as it is. Only installer should be aware (and inform user), that for MUC and PubSub in cluster mode it requires ACS. %kobit Please assign it later back to me, so I will be able to adjust our code. |
|
Artur Hefczyc wrote:
A small comment - in such case installer asks whether enable clustering and allows enabling muc only if ACS is enable if memory serves me right. So if the user later on would change configuration and end up with wrong configuration it would be… somewhat it's fault. |
|
Ok, so we are talking here about clustered mode without ACS right? (Do we really want to maintain this mode....) My question is: how can this happen? Does our web installer allow for this? If it does, then it should make sure non-ACS MUC and PubSub are loaded in such a case. I am confused here, because we are still talking about more or less default behavior. And this is something created by our web installer without any manual intervention. Or maybe the question is what happens with more or less empty configuration file, with some essential settings only. In my opinion an empty config file is for a non-clustered mode. If clustered mode is activated in the installer then installer takes care of correct components to be loaded. If somebody just manually activates clustered mode in the config file, than we do not have to worry about this. As Wojciech pointed out, this is "his fault".
If this is a result of a manual config change by a user, then this is acceptable. I would even go as far as this:
Yes, exactly.
Done. |
|
Now PEP is enabled by default as well. I reviewed description of default behaviour and it will work as described. We only need to work on |
Type |
Bug
|
Priority |
Normal
|
Assignee | |
RedmineID |
4888
|
Version |
tigase-server-8.0.0
|
Spent time |
33h 45m
|
Right now all beans are active by default so without explicit component configuration stating
active=false
it will get loaded if the binary is in the classpath. I think it should be on the opt-in basis (as it was in 7.1.0) instead on the opt-out (except for a couple of default components). Right now for example MUC is explicitly disabled:But any component defining only
@Bean(name = "component")
will get loaded.%kobit - what do you think?