detailed code review for delete-agent-plugin branch

John Sanda jsanda at redhat.com
Tue Dec 7 19:01:41 UTC 2010


On 12/2/10 11:41 AM, Joseph Marques wrote:
> commit 8c091d0f5665967af31bd2fab23e62c4edc4470d
> Author: John Sanda <jsanda at redhat.com>
> Date: Wed Nov 10 17:37:22 2010 -0500
>
> Add deleteAgentPlugins method to PluginManagerBean
>
> InventoryManagerBean now has two versions of markTypesForDeletion, one
> that takes a var-args of resource type ids and the other that takes a
> list of ResourceTypes. The UI has also been updated so that it now has a
> functioning 'DELETE' button for agent plugins.
>
> * looks like you accidentally committed the file
>   
> .../portal-war/0001-Initial-commit-for-CreateResourceHistoryCriteria.jav.patch
> * why keep the function that takes List<ResourceType>?  this method 
> requires
>   fetching more data across the line from the DB (the entire 
> ResourceType entity
>   instead of just having a type id), so I would prefer to see the 
> overload that takes
>   a list of ids stay and remove this one.  the rest of our api 
> attempts to follow this
>   pattern too, where the PKs of the objects to be manipulated are used 
> instead of
>   the objects themselves (except in cases like 
> updateSomeObject(SomeObject)
>   where the object is absolutely needed as part of the method contract) 
markTypesDeleted(List<ResourceType> types) is called from 
PluginManagerBean.deletePlugins(), which is passed a list of plugin ids. 
Here is the relevant portion of that method,

for (Plugin plugin : plugins) {
     if (plugin.getStatus().equals(PluginStatusType.INSTALLED)) {
         long startTime = System.currentTimeMillis();
         List<ResourceType> resourceTypes = 
resourceTypeMgr.getResourceTypesByPlugin(plugin.getName());
         Plugin managedPlugin = entityManager.merge(plugin);
         inventoryMgr.markTypesDeleted(resourceTypes);
         managedPlugin.setStatus(PluginStatusType.DELETED);
         long endTime = System.currentTimeMillis();
         log.debug("Deleted " + plugin + " in " + (endTime - startTime) 
+ " ms");
     } else {
         log.debug("Skipping " + plugin + ". It is already deleted.");
     }
}

I am querying for ResourceTypes by plugin already. When 
inventoryMgr.markTypesDeleted(resourceTypes) is called, the 
ResourceTypes have already been loaded. I didn't see the point in 
duplicating the effort that would have resulted by passing in the ids 
and then reloading the types in markTypesDeleted.


More information about the rhq-devel mailing list