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