Am 2013-03-16 11:43, schrieb Konstantin Kolinko:
2013/3/16 Michael-O <1983-01...@gmx.net>:
Am 2013-03-16 10:46, schrieb Konstantin Kolinko:

2013/3/16 Michael-O <1983-01...@gmx.net>:

Hi,

I'd like to make a string available as an env entry via JNDI. The string
is
static but must be created dynamically at startup time of the webapp.

I have evaluated and implemented a listener which I have added to my
context.xml. It listens for startup and shutdown events. It works pretty
well. Rethinking this makes me ask whether a listener is the right thing
to
do.

Would an object factory a better approach for this?
Did I abuse the listener concept?

Code is available for inspection.


1. Tomcat version =?


I am on Tomcat 6.0.35.


2. Looking at docs,

http://tomcat.apache.org/tomcat-7.0-doc/config/context.html#Environment%20Entries
I do not see factory as an allowed attribute for environment entries.


That was not my intent. I was referring to <Resource> instead of
<Environment>.


3. Do you have control over code that uses the value?


No(t really), it's Logback. I retrieve the value from JNDI in the
logback.xml.


4. Listeners are OK. I wonder though whether JNDI is modifiable or
read-only at that point in time. If it works for you, then it is has
to be modifiable.


It is modifiable. See my implementation please:
http://tinyurl.com/a79jdgt

And the usecase for:
http://tinyurl.com/afu9ppl

I have this in production for almost a year now but decided finally to make
it publically available under ASLv2. That's the reason for my doubts.


Generally I do not see anything substantially wrong with the code.

First of all, thank you for your valuable inspection. It is highly appreciated. I will comment inline.

Some comments on the code:

1. In logback-test.xml  you use ${CONTEXT_NAME}
I think it is a typo, as you are using "contextName" elsewhere.

No, it's not. ${CONTEXT_NAME} is a context scoped, by default predefined variable with the context name. Please see here: http://logback.qos.ch/manual/configuration.html#variableSubstitution

2. In  LogbackContextNameListener.java
Dependency on commons-lang is a bad thing here, as the class has to go
into ${catalina.base}/lib  and it would be better to minimize
dependencies. You should not use libraries that might be included into
webapps, or you will be open to classloader issues, memory leaks etc.
The method you are using is trivial to be implemented inline.

I am fully aware of that and always keep that in mind. In that case, there is no need to worry for the extra dependency. I am shading all necessary classes in an internal package in the JAR. They won't collide with deployed webapps. See http://tinyurl.com/cx7tzne

3. I prefer to use  "constantString.equals(value)" instead of
"value.equals(constantString)" because of nulls. Not an issue here
though. Just style.

I have seen this approach already in several spots in some Apache projects but haven't yet grasped the advantages because I always check input for validity.

4. You are not really changing JNDI. You are changing context
configuration that JNDI is populated from later.
(NamingContextListener registers itself as PropertyChangeListener on
NamingResources and propagates your changes to JNDI)

Wow, that's new. But this "context.getNamingResources().addEnvironment(ce);" does add objects to the context? So, if I understand correctly, startup phase has a staging set of objects from which the final, read-only JNDI context will be build?

5. I would not clear the value on stop event. It causes unnecessary
work (propagating the value to JNDI) and shutdown is supposed to be
fast.

Double assignments during repeated starts should not be an issue, but
if they are then you can call
context.getNamingResources().removeEnvironment(name)  before adding.

Thanks, I will remove the if loop. But reading your comment, I should add a checker on startup which verifies whether that env entry is already set?

Something like:

if (le.getType().equals(Lifecycle.START_EVENT)) {
check for presence;
if yes
 remove;

add value;
}

6. A Factory will not have access to Context. A Listener is better here.

Yes, but it would spit out the same string which will be cached and returned to the app. The outcome would be the same, at least from the app's point of view.


Michael


---------------------------------------------------------------------
To unsubscribe, e-mail: users-unsubscr...@tomcat.apache.org
For additional commands, e-mail: users-h...@tomcat.apache.org

Reply via email to