Re: [digester] plugins module ready for evaluation
On Monday, October 20, 2003, at 10:46 PM, Simon Kitching wrote: On Tue, 2003-10-21 at 10:29, robert burrell donkin wrote: On Sunday, October 5, 2003, at 10:53 PM, Simon Kitching wrote: snip Hmm .. but calling Digester.setLogger probably doesn't override the object known to the LogFactory... What exactly is the purpose of being able to set the Log object used by a class or instance? so the instance used to log by digester can be set programmatically at runtime. this is a useful feature because: 1. it's a very convenient way to bypass the usual commons-logging configuration infrastructure. for example, it's often easier (when debugging) to setLogger programmatically on a particular instance than to reconfigure everything. Easier than changing the logging config file for the appropriate logging implementation (eg the log4j.properties or log4j.xml file)? this allows some things to be done easily that otherwise would be difficult: 1a different instances can use different log systems; 1b different instances can use different log levels (this is particularly useful in complex systems); 2. it allows digester to participate properly in frameworks based on inversion-of-control. (frameworks of this type are configured and controlled in a parent-child fashion. the Log to be used by a digester should be controlled by the component owning digester) This approach requires: (a) The framework to call setLog on each component used by the framework which has a dedicated Log object. Imagine a framework which uses Digester + Net + CLI, where each component works like Digester, requiring the user to call setLog on each one in turn. this approach is advocated by many designers of (usually heavyweight) frameworks. these kinds of framework have their own configuration mechanisms which configure each component in turn. (b) Requiring classes to log via some object stored on some main object in the component, like all classes in the Digester project are required to get the Log object from their owning Digester object. In fact, in many projects this will not be feasable; it only works in Digester because every object of interest happens to have a reference to an owning Digester object. yes and no :) logging to a central object only works when a component has a central object. the alternative for components which consist of loose graphs is typically having setters and getters for each (principal) object. Isn't it much cleaner for the calling framework to set up an appropriate implementation of org.apache.commons.loggging.Logfactory that does whatever the framework wants with respect to logging? If the calling framework wants all logging (regardless of category) to go to one destination, then it creates a LogFactory implementation which returns a single Log object always, and that Log object's implementation writes to the desired destination. 1. this pattern has been found to be very powerful. allowing components to be configure by setting properties is both common and clean. 2. IMHO library code should seek to work flexible with different designs of framework. All those getLog and setLog calls go away. Objects don't need to have references to the master object which holds the Log object to use. Objects log via their own Category, so that filtering can be applied appropriately. i'm very keen about leaving the door open for users to integrate digester into frameworks using IOC and so i'm probably (sooner or later) going to add getters and setters for each log. the real question (as far as i'm concerned) is whether each class needs it' s own logger (with a getter and setter) or whether it's better (in this case) to follow the digester conventions and feed some (or all?) of these into the central digester log. i'd be interested to hear other opinions on this. - robert - To unsubscribe, e-mail: [EMAIL PROTECTED] For additional commands, e-mail: [EMAIL PROTECTED]
Re: [digester] plugins module ready for evaluation
On Tuesday, October 21, 2003, at 08:04 AM, Craig R. McClanahan wrote: snip For classes inside, consistency with past practice overwhelms any principle about the desireability of the practice--otherwise, you totally screw up any existing Digester user that expects his or her current log settings to continue to work when we add additional classes to the package. I'm also going to -1 any proposed class that gratuitously breaks backwards compatiblity (even though it is not an issue that will cause compile-time behavior differences) in this way. It's too late to change this, even if there were good reasons. craig: i'm not sure i'm parsing you correctly here. you mean too late to change the digester conventions, right? (IMHO it's not too late to change the plug-ins stuff.) i'm not sure that i feel quite so strongly about logging symantics as you do. (i probably wouldn't have committed the stuff without changes if i'd thought the issues were black-and-white rather than worthy of debate.) the plugin package is self-contained and any differences in symantics cannot effect users of the other packages. they might have expectations about the way that plugins will handling logging but i'm not sure that it will break the configurations typically used. i'd be happy to see the Rule implementations follow the standard convention and log to Digester.getLog(). (but someone would need to go through and ensure that the calls were correctly prefixed and that the levels were appropriate given that it's a multi-purpose log.) one issue is that there is a *lot* of logging in the plugins packages (that is not intended as a criticism). it seems to me that a lot of this will be needed by users trying to debug their plug-ins but may obfuscate in other cases. i've taken a look and there isn't any real precedent for logging in matching Rules. there may be case for directing logging messages from matching Rules to a separate Log (in the same way that digester has saxLog and Log) since matching Rules get called a *lot* and logging would tend to obfuscate if the problem lies elsewhere. - robert - To unsubscribe, e-mail: [EMAIL PROTECTED] For additional commands, e-mail: [EMAIL PROTECTED]
Re: [digester] plugins module ready for evaluation
On Sunday, October 5, 2003, at 10:53 PM, Simon Kitching wrote: snip Hmm .. but calling Digester.setLogger probably doesn't override the object known to the LogFactory... What exactly is the purpose of being able to set the Log object used by a class or instance? so the instance used to log by digester can be set programmatically at runtime. this is a useful feature because: 1. it's a very convenient way to bypass the usual commons-logging configuration infrastructure. for example, it's often easier (when debugging) to setLogger programmatically on a particular instance than to reconfigure everything. 2. it allows digester to participate properly in frameworks based on inversion-of-control. (frameworks of this type are configured and controlled in a parent-child fashion. the Log to be used by a digester should be controlled by the component owning digester) - robert - To unsubscribe, e-mail: [EMAIL PROTECTED] For additional commands, e-mail: [EMAIL PROTECTED]
Re: [digester] plugins module ready for evaluation
On Sunday, October 5, 2003, at 10:53 PM, Simon Kitching wrote: On Sun, 2003-10-05 at 00:43, robert burrell donkin wrote: snip 7. Logs at the moment, each plugin class uses it's own and the log cannot be set. i'm not in favour of this pattern for several reasons. most rules implementations use the digester log and i'd prefer to switch as many classes as possible to use the log and give the others setters and getters. I disagree with this one. I think having a different logger on each class is one of the most wonderful features of logging, giving much better control of output. Having all logging going via single Log object means that there is no control over what gets logged and what doesn't for different components of Digester. And Digester generates some serious log output when enabled! agreed. the drawback of this approach is that limits the contexts in which digester plug-ins can be used. some frameworks have good reasons to want to be able to control where the output of the log goes. - robert - To unsubscribe, e-mail: [EMAIL PROTECTED] For additional commands, e-mail: [EMAIL PROTECTED]
Re: [digester] plugins module ready for evaluation
On Tue, 2003-10-21 at 10:29, robert burrell donkin wrote: On Sunday, October 5, 2003, at 10:53 PM, Simon Kitching wrote: snip Hmm .. but calling Digester.setLogger probably doesn't override the object known to the LogFactory... What exactly is the purpose of being able to set the Log object used by a class or instance? so the instance used to log by digester can be set programmatically at runtime. this is a useful feature because: 1. it's a very convenient way to bypass the usual commons-logging configuration infrastructure. for example, it's often easier (when debugging) to setLogger programmatically on a particular instance than to reconfigure everything. Easier than changing the logging config file for the appropriate logging implementation (eg the log4j.properties or log4j.xml file)? 2. it allows digester to participate properly in frameworks based on inversion-of-control. (frameworks of this type are configured and controlled in a parent-child fashion. the Log to be used by a digester should be controlled by the component owning digester) This approach requires: (a) The framework to call setLog on each component used by the framework which has a dedicated Log object. Imagine a framework which uses Digester + Net + CLI, where each component works like Digester, requiring the user to call setLog on each one in turn. (b) Requiring classes to log via some object stored on some main object in the component, like all classes in the Digester project are required to get the Log object from their owning Digester object. In fact, in many projects this will not be feasable; it only works in Digester because every object of interest happens to have a reference to an owning Digester object. Isn't it much cleaner for the calling framework to set up an appropriate implementation of org.apache.commons.loggging.Logfactory that does whatever the framework wants with respect to logging? If the calling framework wants all logging (regardless of category) to go to one destination, then it creates a LogFactory implementation which returns a single Log object always, and that Log object's implementation writes to the desired destination. All those getLog and setLog calls go away. Objects don't need to have references to the master object which holds the Log object to use. Objects log via their own Category, so that filtering can be applied appropriately. Thoughts?? Regards, Simon - To unsubscribe, e-mail: [EMAIL PROTECTED] For additional commands, e-mail: [EMAIL PROTECTED]
Re: [digester] plugins module ready for evaluation
Simon Kitching wrote: On Tue, 2003-10-21 at 10:29, robert burrell donkin wrote: On Sunday, October 5, 2003, at 10:53 PM, Simon Kitching wrote: snip Hmm .. but calling Digester.setLogger probably doesn't override the object known to the LogFactory... What exactly is the purpose of being able to set the Log object used by a class or instance? so the instance used to log by digester can be set programmatically at runtime. this is a useful feature because: 1. it's a very convenient way to bypass the usual commons-logging configuration infrastructure. for example, it's often easier (when debugging) to setLogger programmatically on a particular instance than to reconfigure everything. Easier than changing the logging config file for the appropriate logging implementation (eg the log4j.properties or log4j.xml file)? No. Consider a use case like Tomcat, where Digester is used both by the container (to parse server.xml and web.xml files) and also by an application (say, Struts reading struts-config.xml). It's entirely reasonable to for someone to want the digester log output for the container's use of Digester to go to the container's logging hierarchy, and the webapp's use of Digester to log to a webapp hierarchy. 2. it allows digester to participate properly in frameworks based on inversion-of-control. (frameworks of this type are configured and controlled in a parent-child fashion. the Log to be used by a digester should be controlled by the component owning digester) This approach requires: (a) The framework to call setLog on each component used by the framework which has a dedicated Log object. *Allows* not *requires*. If you don't call setLog() you get the default names. Imagine a framework which uses Digester + Net + CLI, where each component works like Digester, requiring the user to call setLog on each one in turn. (b) Requiring classes to log via some object stored on some main object in the component, like all classes in the Digester project are required to get the Log object from their owning Digester object. In fact, in many projects this will not be feasable; it only works in Digester because every object of interest happens to have a reference to an owning Digester object. *Allows* not *requires*. If you don't call setLog() you get the default names. Isn't it much cleaner for the calling framework to set up an appropriate implementation of org.apache.commons.loggging.Logfactory that does whatever the framework wants with respect to logging? If the calling framework wants all logging (regardless of category) to go to one destination, then it creates a LogFactory implementation which returns a single Log object always, and that Log object's implementation writes to the desired destination. Can you show me how you'd solve the problem posed above about Tomcat? Especially in the case where you're running in a restricted environment where you are not allowed to modify the LogFactory in the container itself? All those getLog and setLog calls go away. Objects don't need to have references to the master object which holds the Log object to use. Objects log via their own Category, so that filtering can be applied appropriately. Thoughts?? Regards, Simon If you go back in the commons-dev message archives, you'll find that the reason setLog exists in the first place is because the Avalon folks (who know a little about IoC :-) said they needed it. You seem to be arguing the opposite position, which is kinda interesting. But, at the end of the day, I can't see why having an optional method to set the Log object a Digester instance can use causes you any grief if you never call it. Craig - To unsubscribe, e-mail: [EMAIL PROTECTED] For additional commands, e-mail: [EMAIL PROTECTED]
Re: [digester] plugins module ready for evaluation
On Tue, 2003-10-21 at 11:26, Craig R. McClanahan wrote: Simon Kitching wrote: Regarding your tomcat example, I will have to have a think about this. I'm no expert on complex container architectures, nor on Tomcat, so if you and the Avalon team say the setLog() approach is the only way to go, I'll believe you. Nevertheless, the current approach *feels* wrong to me. I'm just trying to understand why it is necessary to do logging in this way. If you go back in the commons-dev message archives, you'll find that the reason setLog exists in the first place is because the Avalon folks (who know a little about IoC :-) said they needed it. You seem to be arguing the opposite position, which is kinda interesting. But, at the end of the day, I can't see why having an optional method to set the Log object a Digester instance can use causes you any grief if you never call it. The problem is not that the method exists on the Digester class. I'm looking at this from the point-of-view of a contributor to Digester, not a user (hence the dev list address). The problem is that in order to integrate with this solution, every class written for the Digester which needs to log output *must* do: digester.getLog().warn(oops) rather than Logger log = LogFactory.getLogger(ThisClass.class) .. log.warn(oops) The first implication is that with this pattern, *every* object which wants to log output *requires* a reference to whatever object is the holder of the master Log object (a Digester instance in this case). This looks like undesirable coupling to me; I find it difficult to believe that many other projects would be *able* to implement this pattern. For example, the commons-net project: does every object have a reference to some master object that can be used to hold a central Log object? Also see the xmlrules module. This creates new Digester objects to parse the xmlrules input files, but doesn't copy the set Log object. So currently xmlrules is broken with respect to Log behaviour: there is no way for the framework to control the Log object used by the Digester instances created by xmlrules. While this could be considered a bug in xmlrules, I think it shows a flaw in the concept of a centralised Log object attached to some master object; the log control doesn't automatically propagate, but instead requires explicit code. The second implication of the centralised Log object pattern is that there is a single Category for all output generated by all classes in the Digester project. This seems a shame. It may be acceptable for a project of Digester size (20-30 classes), but I presume that there is a limit beyond which this would not be acceptable. Does Tomcat use a single Category to output all of its logging? Regards, Simon - To unsubscribe, e-mail: [EMAIL PROTECTED] For additional commands, e-mail: [EMAIL PROTECTED]
Re: [digester] plugins module ready for evaluation
Simon Kitching wrote: On Tue, 2003-10-21 at 11:26, Craig R. McClanahan wrote: Simon Kitching wrote: Regarding your tomcat example, I will have to have a think about this. I'm no expert on complex container architectures, nor on Tomcat, so if you and the Avalon team say the setLog() approach is the only way to go, I'll believe you. Nevertheless, the current approach *feels* wrong to me. I'm just trying to understand why it is necessary to do logging in this way. If you go back in the commons-dev message archives, you'll find that the reason setLog exists in the first place is because the Avalon folks (who know a little about IoC :-) said they needed it. You seem to be arguing the opposite position, which is kinda interesting. But, at the end of the day, I can't see why having an optional method to set the Log object a Digester instance can use causes you any grief if you never call it. The problem is not that the method exists on the Digester class. I'm looking at this from the point-of-view of a contributor to Digester, not a user (hence the dev list address). The problem is that in order to integrate with this solution, every class written for the Digester which needs to log output *must* do: digester.getLog().warn(oops) rather than Logger log = LogFactory.getLogger(ThisClass.class) .. log.warn(oops) Why? The first approach is only required if you want your log messages to go to the same Log instance that Digester is using. If you don't care, don't bother. And, without a getLog() call, it would not even be *possible* to share the same Log instance, which is what the Avalon folks were complaining about. The first implication is that with this pattern, *every* object which wants to log output *requires* a reference to whatever object is the holder of the master Log object (a Digester instance in this case). Even if this were a *must* requirement, how many objects n the Digester API don't have a Digester instance already? That's right, basically none of them :-). This looks like undesirable coupling to me; I find it difficult to believe that many other projects would be *able* to implement this pattern. For example, the commons-net project: does every object have a reference to some master object that can be used to hold a central Log object? Also see the xmlrules module. This creates new Digester objects to parse the xmlrules input files, but doesn't copy the set Log object. So currently xmlrules is broken with respect to Log behaviour: there is no way for the framework to control the Log object used by the Digester instances created by xmlrules. While this could be considered a bug in xmlrules, I think it shows a flaw in the concept of a centralised Log object attached to some master object; the log control doesn't automatically propagate, but instead requires explicit code. The second implication of the centralised Log object pattern is that there is a single Category for all output generated by all classes in the Digester project. This seems a shame. It may be acceptable for a project of Digester size (20-30 classes), but I presume that there is a limit beyond which this would not be acceptable. Does Tomcat use a single Category to output all of its logging? Centralized logging for Digester is used by the standard Digester classes to minimize the number of log names you have to administer. Whether you utilize it in your own code (or whether your own code is even aware that the possibility exists) is totally up to you. Regards, Simon Craig - To unsubscribe, e-mail: [EMAIL PROTECTED] For additional commands, e-mail: [EMAIL PROTECTED] - To unsubscribe, e-mail: [EMAIL PROTECTED] For additional commands, e-mail: [EMAIL PROTECTED]
Re: [digester] plugins module ready for evaluation
i'm also a bit confused why PluginDeclarationRule throws
ClassNotFoundException's when require attributes are missing from the xml.
this seems a wrong to me. (i've left these for the moment since it's
easier for people to examine the code when it's in cvs.) there are also a
few Exception's thrown. i'd prefer specific subclasses to be thrown since
this allows users (if they wish) to diagnose the original problem.
Well, I chose ClassNotFound partly because the result of these mandatory
attributes being missing is that we don't know which class to
instantiate. Isn't that sort of like ClassNotFound? :-)
Any suggestions for superior options are welcome. I agree it could be
improved. This code was written before the PluginInvalidInputException
class existed; would this be more appropriate?
If there are any raw Exception classes being thrown, these are just
remnants of code before cleanup and should definitely be fixed.
Attached is a patch to fix the locations where ClassNotFoundException
and Exception are being thrown. Class PluginInvalidInputException is
thrown instead.
Patch is based from digester root directory.
Regards,
Simon
Index: src/java/org/apache/commons/digester/plugins/Declaration.java
===
RCS file: /home/cvspublic/jakarta-commons/digester/src/java/org/apache/commons/digester/plugins/Declaration.java,v
retrieving revision 1.2
diff -u -r1.2 Declaration.java
--- src/java/org/apache/commons/digester/plugins/Declaration.java 5 Oct 2003 15:05:38 - 1.2
+++ src/java/org/apache/commons/digester/plugins/Declaration.java 6 Oct 2003 00:00:11 -
@@ -126,8 +126,7 @@
/**
* Constructor.
*/
-public Declaration(String pluginClassName)
-throws ClassNotFoundException {
+public Declaration(String pluginClassName) {
pluginClassName_ = pluginClassName;
}
Index: src/java/org/apache/commons/digester/plugins/PluginCreateRule.java
===
RCS file: /home/cvspublic/jakarta-commons/digester/src/java/org/apache/commons/digester/plugins/PluginCreateRule.java,v
retrieving revision 1.2
diff -u -r1.2 PluginCreateRule.java
--- src/java/org/apache/commons/digester/plugins/PluginCreateRule.java 5 Oct 2003 15:05:30 - 1.2
+++ src/java/org/apache/commons/digester/plugins/PluginCreateRule.java 6 Oct 2003 00:00:11 -
@@ -354,7 +354,7 @@
currDeclaration = pluginManager.getDeclarationById(pluginId);
if (currDeclaration == null) {
-throw new Exception(
+throw new PluginInvalidInputException(
Plugin id [ + pluginId + ] is not defined.);
}
}
Index: src/java/org/apache/commons/digester/plugins/PluginDeclarationRule.java
===
RCS file: /home/cvspublic/jakarta-commons/digester/src/java/org/apache/commons/digester/plugins/PluginDeclarationRule.java,v
retrieving revision 1.2
diff -u -r1.2 PluginDeclarationRule.java
--- src/java/org/apache/commons/digester/plugins/PluginDeclarationRule.java 5 Oct 2003 15:05:30 - 1.2
+++ src/java/org/apache/commons/digester/plugins/PluginDeclarationRule.java 6 Oct 2003 00:00:11 -
@@ -128,13 +128,13 @@
}
if (id == null) {
-throw new ClassNotFoundException(
+throw new PluginInvalidInputException(
mandatory attribute id not present on tag
+ + name + );
}
if (pluginClassName == null) {
-throw new ClassNotFoundException(
+throw new PluginInvalidInputException(
mandatory attribute class not present on tag
+ + name + );
}
@@ -187,7 +187,8 @@
log.debug(plugin redeclaration is identical: ignoring);
return;
} else {
-throw new Exception(Plugin id [ + id + ] is not unique);
+throw new PluginInvalidInputException(
+Plugin id [ + id + ] is not unique);
}
}
@@ -195,7 +196,7 @@
// name. It might be nice someday to allow this but lets keep it
// simple for now.
if (pm.getDeclarationByClass(pluginClassName) != null) {
-throw new Exception(
+throw new PluginInvalidInputException(
Plugin id [ + id + ] maps to class [ + pluginClassName + ]
+ which has already been mapped by some other id.);
}
-
To unsubscribe, e-mail: [EMAIL PROTECTED]
For additional commands, e-mail: [EMAIL PROTECTED]
Re: [digester] plugins module ready for evaluation
On Wed, 2003-10-01 at 17:59, Simon Kitching wrote:
Hi,
Many many moons ago, I proposed a plugins extension for digester.
It is now ready for the world [yes, yes, brave words I know :-]
Follow-up comments:
*
The package overview is accessable via this direct link:
http://issues.apache.org/bugzilla/showattachment.cgi?attach_id=8410
*
I always wanted class PluginRules to be a decorator for other Rules
classes rather than reimplement RulesBase functionality. It hasn't been
possible in the past, because I needed to use a CursorableLinkedList (or
equivalent) to hold the rules. However I realised this morning in the
shower (true!) that given recent changes to the way PluginCreateRule is
implemented, it is now possible to do so.
I have attached an updated (and smaller) code tarfile to the bugzilla
entry http://issues.apache.org/bugzilla/show_bug.cgi?id=23537.
*
Currently, PluginCreateRules creates a new Rules instance each time it
fires, to hold the custom rules associated with whatever concrete class
the user has specified. As it is expected that the same concrete class
will be reused often (just see the examples), a cache could be created
to hold these rules objects. I would rather hold off doing this
optimisation, though, until I know whether plugins will be accepted into
the digester tree or not.
*
Sorry about my coding style creeping into the posted code a little.
In particular, I add underscores to the end of instance variables, like:
private String foo_;
public void setFoo(String foo) { foo_ = foo; }
Habits are hard to break...
*
Calls to log.debug still need to be wrapped in if(debug) for
performance, like in the rest of Digester code.
Cheers,
Simon
-
To unsubscribe, e-mail: [EMAIL PROTECTED]
For additional commands, e-mail: [EMAIL PROTECTED]
