detailed code review for delete-agent-plugin branch

John Sanda jsanda at redhat.com
Tue Dec 7 02:43:26 UTC 2010


Thanks for catching and fixing that!

On 12/6/10 7:41 PM, Joseph Marques wrote:
> One last thing...
>
> On 12/02/2010 11:41 AM, Joseph Marques wrote:
>> commit eded9dd4d1657e25f2f661801ecbc2c11b12214d
>> Author: John Sanda<jsanda at redhat.com>
>> Date: Tue Oct 19 21:32:19 2010 -0400
>>
>> Updating resource type queries to filter on the deleted flag
>>
>> Since the deleted flag is new, all existing queries need to be updated
>> to filter on deleted = false.
>>
>> * I would use "type.deleted = false" instead of
>> "rg.resourceType.deleted = false",
>>    to be sure that Hibernate doesn't unnecessarily create another join
>> to the
>>    rhq_resource_type table.  The where clause that says
>> "rg.resourceType = type"
>>    will likely be translated to "rg0.resource_type_id = rt0.id", but
>> I'm not convinced
>>    Hibernate is smart enough to translate "rg.resourcetype.deleted =
>> false" into
>>    "rt0.deleted = false" as opposed to joining to the rhq_resource_type
>> table yet again,
>>    and then doing "rt1.deleted = false".  And the same goes for the
>> ResourceSearchAssistant.
>> * Notice how are you filtering by "crt2.deleted = false" twice, once
>> in the outer-query
>>    and once in the sub-query.  Adding it once to the outer-query is
>> sufficient, as the same
>>    table alias will be used, thus the rows will already have been
>> filtered for the inner
>>    query.  For "rt2.deleted = false", you can remove that check
>> altogether.  Notice the
>>    "rt2.category = 'PLATFORM'" filter. I don't believe that the
>> platform plugin can be
>>    disabled, as it is a requirement for any other plugin to work. Even
>> if we don't support
>>    native metrics for the platform, we would still fall-back to the
>> Java-based platform.
>>    Thus, since we're filtering "rt2" by platforms, rt2 can never be
>> disabled.
>>
>> -----
>>
>> commit ed4487ba743804f0a7763046be44bd15402ad683
>> Author: John Sanda<jsanda at redhat.com>
>> Date: Wed Oct 20 11:51:59 2010 -0400
>>
>> Tidying up queries based on feedback from code review
>>
>> Modified GroupSearchAssistance and ResourceSearchAssistant to use
>> type.deleted = false instead of rg.resouceType.deleted = false to try
>> and avoid unnecessarily creating another join.
>>
>> Removing superflous checks against deleted flag in queries in
>> ResourceType.java.
>>
>> * each part of the union for the named native queries needs to filter
>> by deleted types.
>>    it was mentioned in the previous code review that adding it once to
>> the outer query
>>    was sufficient, but it was removed from BOTH the inner and outer
>> queries.  in other
>>    words, the named native queries are no longer filtering by the
>> deleted bit at all.
>>
>> -----
> FYI, NamedNativeQuery definitions that use SqlResultSetMappings, must
> include the
> new 'deleted' column data in the projected data.  Since this was broken
> in master, I
> committed a fix (94410dfa) there, so you'll get it the next time you
> merge to your branch.
> _______________________________________________
> rhq-devel mailing list
> rhq-devel at lists.fedorahosted.org
> https://fedorahosted.org/mailman/listinfo/rhq-devel



More information about the rhq-devel mailing list