detailed code review for delete-agent-plugin branch

Joseph Marques jmarques at redhat.com
Tue Dec 7 21:18:00 UTC 2010


On 12/07/2010 02:01 PM, John Sanda wrote:
> 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.
OK, so the logic today says:

deletePlugins(pluginIds) {
    // validation to ensure pluginIds is not missing any dependent pluginIds
    loop(plugin : Plugins) {
       loop(resourceType : notAlreadyDeleted(plugin)) {
          markTypesDeleted(resourcetype);
       }
    }
}

markTypesDeleted(resourceTypes) {
    reosurceIds := getIds(fetchEntities(resourceTypes))
    uninventory(resourceIds)
    typeIds := getIds(getChildResourceTypesForResourceTypes(resourceTypes))
    query-markTypesDeleted(ids)
}

getChildResourceTypesForResourceTypes(resourceTypes) {
    resourceTypeIds := getIds(resourceTypes)
    while (notEmpty(findChildrenOf := moreChildrenToFind())) {
       resourceTypeIds := resourceTypeIds + 
getIds(query-getChildren(findChildrenOf))
    }
   return mapIdsToEntities(resourceTypeIds)
}

-----

But couldn't it instead be written as:

deletePlugins(pluginIds) {
    // validation to ensure pluginIds is not missing any dependent pluginIds
    resourceTypeIds := query-findTypesByPluginIds(pluginIds);
    loop(batch-1000-at-a-item) {
       resourceIds := next-batch(resourceTypeIds)
       uninventory(resourceIds);
    }
    query-markTypesDeleted(resourceTypeIds)
}

This second form attempts to minimize the number of DB roundtrips 
required to implement the requirements.  It exploits the fact that the 
user is deleting a whole plugin sub-graph at a time (all dependent 
pluginIds are already in the list), and looks up all of the 
resourceTypeIds for the passed pluginIds using a single query (instead 
of calling getChildResourceTypesForResourceTypes which requires N+1 
queries where N is the deepest depth of a descendent for some type being 
deleted.  Note this solution also prevents the issue on Oracle where the 
in-clause can't contain more than 1000 items by batching over the 
resources that need to be removed (think of types like Table in the 
Database plugin, which may find more than 1000 instances for a single 
database).


More information about the rhq-devel mailing list