commons-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Siegfried Goeschl <siegfried.goes...@it20one.at>
Subject Re: [Exec] Change needed to Executor interface
Date Sun, 20 Apr 2008 21:32:49 GMT
Hi folks,

looking at the code before falling asleep ....

Regarding the exit code handling we have two driving forces

1) determine a success/failure depending on the operating system 
(DefaultExecutor.isFailure())

2) overwrite the OS-specific semantic (DefaultExecutor.isSuccess()) - I 
have applications always returning '1' and worked with other 
applications that return bit fields to indicate success/warning/errors. 
The problem with DefaultExecutor is being too helpful - the default 
behavior is to throw an exception so the client usually never see the 
failure code.

Looking at the implementation

a) As sebb pointed out the success/failure detection is a little bit odd 
under VMS compared to other OS therefore a 'isFailure()' method should 
go into the CommandLauncher interface with a special implementation in 
VmsCommandLauncher. The DefaultExecutor holds a reference to a 
CommandLauncher so it can delegate the check to the CommandLauncher 
instance. This would remove this obfuscated 'instanceof' hack (asking 
for OS.isFamilyOpenVms() is pretty much the same as 'if (launher 
instanceOf VmsCommandLauncher)')

b) Having a isSucess() and isFailure() method is confusing - there 
should be only one method name for checking the same one thing whereas I 
lean towards isFailure() since this is probably there more common check 
in application code.

c) Therefore I wold expose a 'boolean Executor.isFailure(int exitValue)' 
to be implemented in DefaultExecutor which would also handle 
user-specific overwriting of  exit code checking

In short without technobabble (see http://www.pathcom.com/~boby/babble.htm)

+) expose a CommandLauncher.isFailure(int) and implement it in the 
implementation classes such as VmsCommandLaunher
+) expose a Executor.isFailure(int) to be implemented in DefaultExecutor
+) fix the regression tests

Any opinions out there

Siegfried Goeschl


Matt Benson wrote:
> --- sebb <sebbaz@gmail.com> wrote:
>
>   
>> On 17/04/2008, Matt Benson <gudnabrsam@yahoo.com>
>> wrote:
>>     
>>>  --- Siegfried Goeschl <sgoeschl@gmx.at> wrote:
>>>
>>>  > Hi sebb,
>>>  >
>>>  > thanks for your input and I'm sorry for my late
>>>  > response - I will look
>>>  > at the stuff during the weekend
>>>  >
>>>  > Siegfried Goeschl
>>>  >
>>>  > sebb wrote:
>>>  > > VMS testing has revealed that the
>>>  > DefaultExecutorTest class assumes
>>>  > > that 0 = success, and 1 = failure.
>>>  > >
>>>  > > This is not the case for all OSes - VMS
>>>       
>> regards
>>     
>>>  > odd numbers as
>>>  > > successful and even ones as failures.
>>>  > >
>>>  > > So I think the test needs to use a more
>>>       
>> versatile
>>     
>>>  > means of checking statuses.
>>>  > >
>>>  > > There is a method in DefaultExecutor
>>>  > >    public static boolean isFailure(final int
>>>  > exitValue)
>>>  > > However, it is static, so cannot be added to
>>>       
>> the
>>     
>>>  > Executor interface.
>>>  > >
>>>  > > Any objections if I make it non-static and
>>>       
>> add it
>>     
>>>  > to the interface?
>>>  > > This will allow the test suite to correctly
>>>       
>> check
>>     
>>>  > for success/failure.
>>>  > >
>>>  > > I suspect the private boolean isSuccess(final
>>>       
>> int
>>     
>>>  > exitValue) method
>>>  > > may need updating for VMS. And I don't
>>>       
>> understand
>>     
>>>  > why it returns true
>>>  > > if there is no exitValues array - perhaps it
>>>  > should return !
>>>  > > isFailure(exitValue) ? Or throw an error of
>>>       
>> some
>>     
>>>  > kind?
>>>
>>>
>>> If the test case exists simply to test
>>>  DefaultExecutor, why not explicitly refer to
>>>  DefaultExecutor.isFailure()?
>>>       
>> Yes, that's also possible.
>>
>>     
>>>  Or move the static
>>>  method into an abstract BaseExecutor superclass
>>>       
>> that
>>     
>>>  hypothetical other Executor implementations could
>>>       
>> use
>>     
>>>  for the same purpose, and refer to the method
>>>       
>> there?
>>
>> Likewise.
>>
>> However, I think any Executor implementation is
>> going to need to
>> provide an isFailure() - or perhaps better an
>> isSuccess() - method for
>> callers to be able to check if the execute has
>> succeeded or not.
>>     
>
> Okay, so compromise = add isSuccess() to the interface
> and implement in an abstract base class that
> encapsulates the VMS exception to the common 0 ->
> success rule?  :)
>
> -Matt
>
>   
>>>  -Matt
>>>
>>>
>>>  > >
>>>  > >
>>>  >
>>>
>>>       
> ---------------------------------------------------------------------
>   
>>>  > > To unsubscribe, e-mail:
>>>  > dev-unsubscribe@commons.apache.org
>>>  > > For additional commands, e-mail:
>>>  > dev-help@commons.apache.org
>>>  > >
>>>  > >
>>>  > >
>>>  > >
>>>  >
>>>  >
>>>
>>>       
> ---------------------------------------------------------------------
>   
>>>  > To unsubscribe, e-mail:
>>>  > dev-unsubscribe@commons.apache.org
>>>  > For additional commands, e-mail:
>>>  > dev-help@commons.apache.org
>>>  >
>>>  >
>>>
>>>
>>>
>>>
>>>      
>>>       
> ____________________________________________________________________________________
>   
>>>  Be a better friend, newshound, and
>>>  know-it-all with Yahoo! Mobile.  Try it now. 
>>>       
> http://mobile.yahoo.com/;_ylt=Ahu06i62sR8HDtDypao8Wcj9tAcJ
>   
>>>
>>>       
> ---------------------------------------------------------------------
>   
>>>  To unsubscribe, e-mail:
>>>       
>> dev-unsubscribe@commons.apache.org
>>     
>>>  For additional commands, e-mail:
>>>       
>> dev-help@commons.apache.org
>>     
>>>       
>>     
> ---------------------------------------------------------------------
>   
>> To unsubscribe, e-mail:
>> dev-unsubscribe@commons.apache.org
>> For additional commands, e-mail:
>> dev-help@commons.apache.org
>>
>>
>>     
>
>
>
>       ____________________________________________________________________________________
> Be a better friend, newshound, and 
> know-it-all with Yahoo! Mobile.  Try it now.  http://mobile.yahoo.com/;_ylt=Ahu06i62sR8HDtDypao8Wcj9tAcJ
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
> For additional commands, e-mail: dev-help@commons.apache.org
>
>
>
>   

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org


Mime
View raw message