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