Mark,
Thank you for taking the lead on this. Comments in line.
At 22:24 28.05.2002 -0700, [EMAIL PROTECTED] wrote:
>1) The purpose of the reconfigurator is to primarily "watch" or "monitor" a
>"source" for some indication that said "source" has been changed or updated
>with new/different configuration data.
>2) Once the change in state has been detected, the reconfigurator is
>implemented and configured with enough information to reconfigure log4j
>using the new/updated configuration data.
>3) The reconfigurator implementation does not deal with the specific format
>of the configuration data or even the specific reconfiguration of log4j. It
>delegates this responsibility to the Configurator class, where it belongs.
>This means that the reconfigurator design should support any Configurator
>implementation. The most the reconfigurator knows about the source of the
>new configuration data is how to locate it and hand it off to the
>Configurator.
>
>4) The reconfigurator design should provide for better control over when a
>reconfigurator is active and allows for reconfigurators to be stopped and
>even restarted.
Yes on all 4 accounts. :-)
[snip]
>When reviewing this design and code, it might be useful to keep the
>following items in mind:
>
>1) How easy would it be for a developer to pick up the interface and/or base
>class and use it to develop their own reconfigurator? Does the base class
>help or hinder in this regard?
>2) Knowing that allowing reconfigurators to be created/started within
>settings in a configuration file, does this design lend itself to this goal?
>Should the current set of active reconfigurators "live" in a known location?
>Where would this location be? How can this be strictly enforced?
>3) Is Reconfigurator the right name for this functionality?
How about "Scout" or "ScriptScout"? A ScriptScout scouts for changes
in configuration scripts. Scout is both a noun and a verb, so method
names like startScouting or stopScouting make sense.
I propose ScriptScout as the interface name and ScriptScoutBase as the
base class implemented by FileScout, HTTPScout, SocketScout
subclasses.
>Watchdog.java----------------------------------------------
[snip]
No comments on the interface. It looks complete and perfectly reasonable.
>WatchdogBase.java----------------------------------------------
>
>/**
[snip]
> @author Mark Womack
>*/
>public abstract class WatchdogBase implements Watchdog, Runnable {
>
> /**
> The thread running this watchdog. */
> protected Thread watchdogThread;
>
> /**
> True if this watchdog is running. */
> private boolean running = false;
>
> /**
> The name of this watchdog. */
> protected String name;
>
> /**
> The class name of the configurator to use when reconfigurating. */
> protected String configuratorClassName;
>
> /**
> Returns true if this watchdog is currently running. */
> public
> synchronized
> boolean isRunning() {
> return running;
> }
>
> /**
> Set the name of this watchdog. The name is used by other
> components to identify this watchdog. */
> public
> void setName(String _name) {
> name = _name;
> }
>
> /**
> Get the name of this watchdog. The name uniquely identifies the
> watchdog. */
> public
> String getName() {
> return name;
> }
>
> /**
> Sets the configurator class used for reconfiguration. */
> public
> void setConfigurator(String _configuratorClassName) {
> configuratorClassName = _configuratorClassName;
> }
>
> /**
> Gets the configurator class used for reconfiguration. */
> public
> String getConfigurator() {
> return configuratorClassName;
> }
>
> /**
> Starts this watchdog. After calling this method the
> watchdog will be active. */
> public
> synchronized
> void startWatching() {
> // if not already running
> if (!running) {
> // create a thread to run this
> watchdogThread = new Thread(this);
>
> // setup the thread
> watchdogThread.setName(this.name);
> watchdogThread.setDaemon(true);
>
> LogLog.debug(this.getName() + " watchdog starting");
>
> // start the thread
> running = true;
> watchdogThread.start();
> }
> }
>
> /**
> Stops this watchdog. After calling this method the
> watchdog will become inactive, but it is not guaranteed
> to be immediately inactive. It may take time for the
> thread to be interupted and exited. */
> public
> synchronized
> void stopWatching() {
> // if already running
> if (running) {
> LogLog.debug(this.getName() + " watchdog stopping");
>
> // indicate we are no longer supposed to run
> running = false;
>
> // interrupt the thread if it is sleeping
> watchdogThread.interrupt();
The Thread.interrupt() method is evil. It is also absolutely useless
except for indulging in excruciatingly complex interrupt/notify race
conditions. Thread.interrupt() makes it unnecessarily complicated to
write robust multi-threaded code. As you might except, log4j does not
contain any Thread.interrupt() method calls.
See for example:
http://www.profcon.com/cargill/jgf/9805/threadingIssues.html
http://elvis.rowan.edu/~hartley/SIGCSE/Workshop/raceConditions.html
In his book "Concurrent Programming", Stephen Hartley a Computer
Science professor gives an example of a Semaphore implemented in Java
using monitors. His implementation contains a interrupt/notify related
race condition. It will take him another 4 iterations until he gets it
right (after the book is published) that is until someone finds a bug
in the 4th iteration.
For more info on the book see:
http://www.mcs.drexel.edu/~shartley/ConcProgJava/
It's a good book by the way. If the Thread.interrupt method gives an
Computer Science professor and an expert in the field such hard time, I
don't think we should attempt to be "smart". Thread.interrupt is not
needed. Nice and simple. End of story.
Just remove the "watchdogThread.interrupt();" statement and your code
will continue to run just as well.
> // lose the reference to this thread for gc
> watchdogThread = null;
> }
> }
>
> /**
> Implementation required by the Runnable interface. Calls the
> setup() method, then enters a loop that will run until the
> boolean member running becomes false. The loop calls the
> checkForReconfiguration() method, and if this method returns
> true, the reconfigure() method is called. When this loop is
> exited, the cleanup() method is called before this method
> is exited. */
> public
> void run()
>
> try {
> // setup before starting loop
> setup();
>
> LogLog.debug(this.getName() + " watchdog now active");
>
> // while we are supposed to be running
> while (isRunning()) {
>
> // check for new reconfiguration data
> if (checkForReconfiguration() && isRunning()) {
>
> LogLog.debug(this.getName() + " watchdog reconfiguring");
>
> reconfigure();
> }
> }
> }
You might want to modify the loop as follows:
while(isRunning()) {
if(reconfigurationNeeded()) {
reconfigure();
}
delay();
}
I renamed checkForReconfiguration as reconfigurationNeeded. The
isRunning check in the if statement is correct but not absolutely
required. The delay method allows a subclass to introduce a delay if
it needs to have a delay, e.g. FileScout, HTTPScout but not
SocketScout.
> finally
>
> // cleanup after finishing loop
> cleanup();
> }
>
> LogLog.debug(this.name + " watchdog now inactive");
> }
[snip]
--
Ceki
SUICIDE BOMBING - A CRIME AGAINST HUMANITY
Sign the petition: http://www.petitiononline.com/1234567b
I am signer number 22106. What is your number?
--
To unsubscribe, e-mail: <mailto:[EMAIL PROTECTED]>
For additional commands, e-mail: <mailto:[EMAIL PROTECTED]>
