[jboss-dev-forums] [Design of EJB 3.0] - Re: EJBTHREE-1396 MockServer must report startup / shutdown

[ALRubinger]

Figured you're waiting on pins and needles for my opinion. ;)

* Methods like "hasMonitoringSupport" violate OO practices; this is what inheritance is for.  You can ensure that a MockServer has this support by removing "addMonitoringSupport(int port)" in favor of another argument to a MonitorableMockServer constructor if indeed being "Monitorable" is a separate add-on capability (which might even come with its own interface).

* Constants like SERVER_STARTING should be marked "final"?

* MockServerMonitor has hardcoded "localhost" used by the ServerSocket.  If we've gotta specify a port, might as well specify a host bind address (to bind to 2 unique hosts using same port for example)

* In MockServerMonitor.waitForServerStartup(), there's a "while not started" loop.  Let's throw a Thread.sleep(100) or something in there as to relax the CPU a bit

* RemoteAccessTestCase now has "mockServerMonitor.waitForServerStartup();", which will block indefinitely - what if server does not start?  Recommend making a "private volatile boolean remoteServerStarted" field, spawning off the check to "waitForServerStartup" into a j.u.c.FutureTask, and then call "getResult()" on it with a sensible timeout value.  The FutureTask's job would be to set the "remoteServerStarted" to true, and then the RemoteAccessTestCase can poll for that.  In the case of a timeout of the FutureTask, throw an exception and fail the test.

* I put a static instance of MockServer *in* MockServer?  It'd make me smile if you could remove that so we can actually start up more than one MockServer.  This shouldn't be a Singleton, and even if it was, that's just bad implementation.

S,
ALR

View the original post : http://www.jboss.com/index.html?module=bb&op=viewtopic&p=4187137#4187137

Reply to the post : http://www.jboss.com/index.html?module=bb&op=posting&mode=reply&p=4187137