On Nov 5, 2007, at 6:55 PM, Paul Smith wrote:
I'd like to propose a change to SocketHubAppender code to allow it
automatically choose a free port on the local host if the Port
property is configured with 0.
This will allow the Zeroconf module to be more useful, and allow
simpler configuration for multiple applications on the same host.
We have an open feature request (see [1]) to allow this, and the
only simple way I can get this to work is to change
SocketHubAppender (see attached patch).
In summary, I've moved the ServerSocket initialisation from the
ServerMonitor.run() method to the ServerMonitor constructor, and
when Port=0, check what port is actually configured, and modify the
parent SocketHubAppender instance port variable so that getPort()
exposes the real port in use.
Sub-classes can then inspect that property after the call to
super.activateOptions(). This allows ZeroconfSocketHubAppender to
broadcast the correct port in use (it would have broadcast 0
without this change; not very useful).
This doesn't appear to break any of the log4j unit tests, and I've
added unit tests to the Zeroconf module (see second patch) that
confirm behaviour. I've also made the Zeroconf module aware of the
fact that it might not be using an appropriate log4j version, given
that this feature would/might only be present in log4j 1.2.16.
Prior versions of log4j will work fine still, but the automatic
port configuration won't work (it throws an
UnsupportedOperationException if it detects this).
cheers,
Paul
[1] http://issues.apache.org/bugzilla/show_bug.cgi?id=43295
<log4j_sockethubappender.patch>
<log4j_zeroconf.patch>
I've only briefly looked at the SocketHubAppender patch. A few
comments/questions:
There are a quite a few whitespace only changes that make it
difficult to review only the code that changed. I'm guessing it is
your IDE trying to be helpful, but it would be nice if you could
change the configuration not to do that.
I think I'd be more comfortable to add a distinct accessor for the
bound port instead of overwriting the port specification. If you did
that, you could distinguish between the case where you did not
specify a port and one was assigned versus explicitly specifying that
port. If you did that, I think you could avoid moving the socket
initialization from the run method to the constructor.
Okay, I'm about ready to fall over, but I looked at zeroconf and see
your motivation for moving the binding onto the main thread so you
continue the set up in ZeroConfSocketHubAppender.activateOptions.
However instead of changing the behavior, I'd be more inclined to
make a few changes to SocketHubAppender to make it more amenable to
extension but keep the old behavior. I'd suggest:
Moving ServerMonitor out to a separate public class.
Move the self-start in ServerMonitor to SocketHubAppender.startServer.
Make SocketHubAppender.startServer protected and overload it in
ZeroConf.
Add a protected SocketHubAppender.stopServer and move the
ServerMonitor.stopMonitor code there.
---------------------------------------------------------------------
To unsubscribe, e-mail: [EMAIL PROTECTED]
For additional commands, e-mail: [EMAIL PROTECTED]
