Done.

As for the concept of dropping private methods entirely. I am not in favor of this idea. Each public methods becomes a part of the software API. If you make everything public the API becomes very messy and chaotic. This actually works against extensibility.

I think API should be simple and flexible and well thought.

Applied in changeset r3003.

sorry to be a pain, but do you really think that #getDomains should put the domain value in the corresponding session? everybody who overwrites #getDomains would have to know and implement this. i think the contract of this method should rather be return the domain value, nothing else. care to comment?

Heh, here you go. First you want to change a method from private to public and now you are not happy with the method implementation because it may be overridden. :-)

Please note, this method is called very often, for each packet processed for this user. You really want this method to perform quickly and reduce I/O to absolute minimum. Therefore we have to avoid DB calls as much as possible.

So the current implementation stores the data in the user session data, in memory to access them very quickly. Only when the method is called for the first time and there is no data in the cache (user session object) it requests data from the database.

The same happens to many other user data in Tigase server, roster, privacy lists etc...

I think the method should not be overridden, it should only be called to retrieve information it is supposed to retrieve. And how the method does it should not be important. However, is somebody really wants to override it, he must know and understand what he is doing.

Artur Hefczyc wrote:

Heh, here you go. First you want to change a method from private to public and now you are not happy with the method implementation because it may be overridden. :-)

more precisely: i wanted the methods to be public, but they contain more logic than a developer would expect. IMHO, #getDomains should just do nothing but do its part to get a DOMAIN value. storing it in the session should be done by the surrounding framework.

so ideally, things would look like this:

if (null == domains) {

domains = getDomains();

session.put(...);

}

this way i could override #getDomains without caring about internal logic

Please note, this method is called very often, for each packet processed for this user. You really want this method to perform quickly and reduce I/O to absolute minimum. Therefore we have to avoid DB calls as much as possible.

got that. i was wondering whether i should used DOMAIN.LIST but it seems to eat too much CPU at this point.

So the current implementation stores the data in the user session data, in memory to access them very quickly. Only when the method is called for the first time and there is no data in the cache (user session object) it requests data from the database.

understand that. that's why i implemented a class LocalDomainFilter which applies DOMAIN.LOCAL to every session, not caring whether it's been set before. for this purpose, i just needed to override #getDomains with a 'return DOMAINS.LOCAL'. i don't see why i have to pay attention to session handling here.

The same happens to many other user data in Tigase server, roster, privacy lists etc...

I think the method should not be overridden, it should only be called to retrieve information it is supposed to retrieve. And how the method does it should not be important. However, is somebody really wants to override it, he must know and understand what he is doing.

i guess we're really talking about the same thing, only that the implementation needs a final tweak ;)

A side note. Please do not take my comments personally. I really enjoy discussion and talking about low level technical aspects of the code implementation. I am not trying to convince you that the current code is perfectly correct, I am only presenting my view.

tom quas wrote:

Artur Hefczyc wrote:

Heh, here you go. First you want to change a method from private to public and now you are not happy with the method implementation because it may be overridden. :-)

more precisely: i wanted the methods to be public, but they contain more logic than a developer would expect. IMHO, #getDomains should just do nothing but do its part to get a DOMAIN value.

This is precisely what it does. I think you are too concerned about the internal logic and I do not understand why do you think anybody who uses it might be concerned about it too. Even if you override the method and call super.getDomains(...) you do not have to worry about the method storing data in user's session cache or anything.

The method just returns what you need and all the code inside is about "how to return the data you need". So, as I see this, the getDomains method "ust do nothing but do its part to get a DOMAIN value". Storing data in user session is only for this method internal use, as a cache to quickly access the data, the next time it is called. No code or logic outside the method has to be concerned about it or know anything about it.

storing it in the session should be done by the surrounding framework.

Well, why you do not consider the method a part of "the surrounding framework"?

so ideally, things would look like this:

if (null == domains) {

domains = getDomains();

session.put(...);

}

Not, this is incorrect. This particular piece of data in the user session are need and used by the method. And should be only be used by this method. So only this method should be concerned about putting or retrieving them from the user session.

this way i could override #getDomains without caring about internal logic

I have an impression you misunderstand the code here.

Please note, this method is called very often, for each packet processed for this user. You really want this method to perform quickly and reduce I/O to absolute minimum. Therefore we have to avoid DB calls as much as possible.

got that. i was wondering whether i should used DOMAIN.LIST but it seems to eat too much CPU at this point.

Programing on the server side is mostly about optimizations of the code and resources usage.

So the current implementation stores the data in the user session data, in memory to access them very quickly. Only when the method is called for the first time and there is no data in the cache (user session object) it requests data from the database.

understand that. that's why i implemented a class LocalDomainFilter which applies DOMAIN.LOCAL to every session, not caring whether it's been set before. for this purpose, i just needed to override #getDomains with a 'return DOMAINS.LOCAL'. i don't see why i have to pay attention to session handling here.

Well you do not have to pay attention. It's all internal method logic, it does not concern you.

Artur Hefczyc wrote:

A side note. Please do not take my comments personally. I really enjoy discussion and talking about low level technical aspects of the code implementation. I am not trying to convince you that the current code is perfectly correct, I am only presenting my view.

confirmed. same holds true for my comments. it's the subject that matters.

This is precisely what it does. I think you are too concerned about the internal logic and I do not understand why do you think anybody who uses it might be concerned about it too. Even if you override the method and call super.getDomains(...) you do not have to worry about the method storing data in user's session cache or anything.

The method just returns what you need and all the code inside is about "how to return the data you need". So, as I see this, the getDomains method "ust do nothing but do its part to get a DOMAIN value". Storing data in user session is only for this method internal use, as a cache to quickly access the data, the next time it is called. No code or logic outside the method has to be concerned about it or know anything about it.

storing it in the session should be done by the surrounding framework.

Well, why you do not consider the method a part of "the surrounding framework"?

we still don't agree here; let's consider this use case: i create class LocalDomainFilter and override #getDomains like this:

@Override

getDomains() {

return DOMAINS.LOCAL;

}

this is how i ideally define the contract between the server code and extensions out of the server's scope. now, we both agree that this will break the caching mechanism, which is bad for more complex lookup cases.

so all i'm saying is that overriding #getDomains should be as simple as doing the above, while the original server code needs to provide an additional layer of abstraction including the caching. here's what i'd do with the current code:

1. rename #getDomains to #getAndCacheDomains // this describes the contained functionality better

2. extract the code responsible to look up domain values into a separate method #getDomains

3. assign private scope to #getAndCacheDomains

4. assign public scope to #getDomains

the above scheme just simplifies developing extensions to tigase. as it stands today, i need to understand too much of the internals in order to extend it and not break things like caching. of course, you're free to disagree and leave everything the way it is.

thank you for taking the time to explain things.

Well, why you do not consider the method a part of "the surrounding framework"?

we still don't agree here; let's consider this use case: i create class LocalDomainFilter and override #getDomains like this:

@Override

getDomains() {

return DOMAINS.LOCAL;

}

this is how i ideally define the contract between the server code and extensions out of the server's scope. now, we both agree that this will break the caching mechanism, which is bad for more complex lookup cases.

It appears to be some misunderstanding here. I don't see how it breaks the caching mechanism. Your version of the method, simply, does not cache anything because it does not need to. How is this breaking anything?

My method retrieves data differently, it uses in memory cache instead of database if possible. Yours does it differently, and since your method overrides mine and your method is used instead of mine, there is no problem or harm. Caching is not used because it is not needed.

I have an impression that you think that storing data in a cache by my version of the method serves some other purpose than the method itself. It does not. If the method is not used, the cache is not needed.

so all i'm saying is that overriding #getDomains should be as simple as doing the above,

It is, it really is.

while the original server code needs to provide an additional layer of abstraction including the caching.

No, it does not. The caching is used and needed internally by the method only. If you have your own version of the method, you may need or may not need some sort of caching. And you may want or need to implement caching differently.

here's what i'd do with the current code:

1. rename #getDomains to #getAndCacheDomains // this describes the contained functionality better

2. extract the code responsible to look up domain values into a separate method #getDomains

3. assign private scope to #getAndCacheDomains

4. assign public scope to #getDomains

the above scheme just simplifies developing extensions to tigase. as it stands today, i need to understand too much of the internals in order to extend it and not break things like caching. of course, you're free to disagree and leave everything the way it is.

And what if after some time you find out that your overridden getDomains method is too slow and you have to speed it up somehow? You may decide to use some kind of caching for it or you may decide to improve performance in a different way. Then you have a more complex internal logic and you are at the beginning of the discussion.

I really believe this all works exactly as it should and, more over, exactly as you expect it and want it to work. I repeat, the caching used in my method is used only internally by this method.

got it – eventually ;)

you're saying caching is essential for particluar implementations, whereas i'm saying caching should be built-in generally. just two different views.

i started the discussion mainly because i wasn't sure whether some other code would rely on the domain value being stored in the session already.

glad we could clarify things.

tom quas wrote:

got it – eventually ;)

you're saying caching is essential for particluar implementations, whereas i'm saying caching should be built-in generally. just two different views.

Actually both are true and not really different views. (I know I tend to not agree with you ;-) ).

1. Caching is available as a general feature to all plugins in form of the user session object which allows any plugin to store some data in memory

2. There are many particular implementations and plugins which use the caching mechanism for local needs

i started the discussion mainly because i wasn't sure whether some other code would rely on the domain value being stored in the session already.

That would be bad indeed, bad design and bad implementation.