detailed code review for delete-agent-plugin branch

John Sanda jsanda at redhat.com
Tue Dec 7 02:49:42 UTC 2010


I spent some time earlier this afternoon/evening to retest to figure out 
what was going on. Initially when I had StaleTypeException inheriting 
from InvalidInventoryReportException I was careful to add the exception 
to the throws clause of DiscoveryServerService, but I do not think I 
added it to DiscoveryServerServiceImpl. I am going to go back and change 
StaleTypeException to inherit from InvalidInventoryReportException.

On 12/6/10 2:18 PM, Joseph Marques wrote:
> On 12/06/2010 01:47 PM, John Sanda wrote:
>> On 12/2/10 11:41 AM, Joseph Marques wrote:
>>>
>>>
>>> Reject incoming reports (on the server) that contain stale resource 
>>> types
>>>
>>> This commit contains two changes needed for BZ 549852. First, filtering
>>> logic has been added in DiscoveryBossBean.mergeInventoryReport to 
>>> reject
>>> inventory reports that include any resource types that have been marked
>>> for deletion. A report is rejected by throwing a StaleTypeException.
>>>
>>> The second change is handling the StaleTypeException in the agent 
>>> and in
>>> the plugin container. A new listener interface, RebootRequestListener,
>>> has been added to the PC. When InventoryManager catches a
>>> StaleTypeException, it notifies the listener that a reboot is needed.
>>> This listener is the agent. The agent shuts down the PC, clears out the
>>> data directory, and then restarts the PC.
>>>
>>> * You changed StaleTypeException to not inherit from 
>>> InvalidInventoryReportException,
>>>   but you could have left this and the exception handling still 
>>> would have worked as
>>>   long as you caught StaleTypeException first and then 
>>> InvalidInventoryReportException.
>>> * You probably did this because you saw failures in 
>>> ResourceUpgradeFailureHandlingTest
>>>   and DiscoveryTestBean, which catch the exceptions in the reverse 
>>> order.
>>>
>> Actually, I could not do as you suggested and I did not make changes 
>> due to any test failures. The problem stems from logic in 
>> RemotePojoInvocationCommandService.getWrappedException(Throwable e, 
>> Method m). The javadocs for this method state,
>>
>>    Examines the given exception and if it matches a type that can be
>>    thrown from the given method, it is returned as-is; otherwise, it is
>>    wrapped in an exception that is suitable for sending to a remote
>>    client. This method is used because we want to ensure that the given
>>    exception can be processed on the other side....
>>
>> On line 292 we have,
>>
>> Class<?>[] declared_exceptions = method.getExceptionTypes();
>>
>> When StaleTypeException inherited from 
>> InvalidInventoryReportException, method.getExceptionTypes() only 
>> included InvalidInventoryReportException in the returned array which 
>> makes sense since StaleTypeException was a type of 
>> InvalidInventoryReportException. From the compiler's perspective, 
>> declaring StaleTypeException in the throws clause was superfluous. 
>
> That seems strange.  I coded a quick example locally to check this 
> behavior:
>
> import java.lang.reflect.Method;
>
> public class ExceptionTest {
>     public void dummyRegular() throws ChildException, ParentException {}
>     public void dummyInverse() throws ParentException, ChildException {}
>
>     public static void main(String[] args) {
>         Method[] methods = ExceptionTest.class.getMethods();
>         for (Method nextMethod : methods) {
>             System.out.println("Method " + nextMethod.getName());
>             Class<?>[] exceptionTypes = nextMethod.getExceptionTypes();
>             for (Class<?> nextExceptionType : exceptionTypes) {
>                 System.out.println("   ExceptionType: " + 
> nextExceptionType);
>             }
>         }
>     }
> }
>
> class ParentException extends RuntimeException {}
> class ChildException extends ParentException {}
>
> And it prints:
>
> Method dummyRegular
>    ExceptionType: class ChildException
>    ExceptionType: class ParentException
> Method dummyInverse
>    ExceptionType: class ParentException
>    ExceptionType: class ChildException
>
> So as long as you specify the exception type in the throws clause, 
> reflection will return to you all exceptions declared to be thrown.  I 
> surmise that you hadn't explicitly declared the StaleTypeException in 
> the throws clause (because it's not specifically necessary given Java 
> language rules), and that's why it didn't show up in the call to 
> getExceptionTypes().
>
>> A few lines down getWrappedException iterates through 
>> delcared_exceptions and performs the following check on line 302,
>>
>> if (declared_exception.getName().equals(e.getClass().getName()))
>>
>> When that condition holds true the exception is serialized as is and 
>> sent to the agent, but it was not true when StaleTypeException 
>> inherited from InvalidInventoryReportException. If line 302 were 
>> written as,
>>
>> if (delcared_exception.isInstance(e))
>
> Actually, this code change is not possible.  Let's say a method 
> declares "throws Throwable", then it would match every single class 
> type ever passed to it, but it's not guaranteed that the agent has the 
> definition of that class.  This is why the code today does an exact match.
>
>>
>> then I could have left StaleTypeException as a subtype of 
>> InvalidInventoryReportException.
>>
>> - John
>>
>>
>> _______________________________________________
>> rhq-devel mailing list
>> rhq-devel at lists.fedorahosted.org
>> https://fedorahosted.org/mailman/listinfo/rhq-devel
>
>
> _______________________________________________
> rhq-devel mailing list
> rhq-devel at lists.fedorahosted.org
> https://fedorahosted.org/mailman/listinfo/rhq-devel

-------------- next part --------------
An HTML attachment was scrubbed...
URL: https://fedorahosted.org/pipermail/rhq-devel/attachments/20101206/7f7b32e1/attachment.html 


More information about the rhq-devel mailing list