liron aravot has posted comments on this change.

Change subject: core: introducing OvfAutoUpdate
......................................................................


Patch Set 18: (6 inline comments)

....................................................
File backend/manager/dbscripts/vms_sp.sql
Line 720:    AS $procedure$
Line 721: BEGIN
Line 722: RETURN QUERY SELECT vm.*
Line 723:              FROM vms vm
Line 724:              WHERE vm.vm_guid in (SELECT ID from 
fnSplitterUuid(v_vms_ids));
all over the code, 
select X forces us to return only one column from the splitter method, why do 
we need to have such contract between the implementation and the usage? i 
prefer to leave it like that for uniformity.
Line 725: END; $procedure$
Line 726: LANGUAGE plpgsql;
Line 727: 
Line 728: 


....................................................
File 
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/OvfDataUpdater.java
Line 277:     protected void buildMetadataDictionaryForTemplate(VmTemplate 
template , Map<Guid, KeyValuePairCompat<String, List<Guid>>> metaDictionary){
Line 278:         List<DiskImage> allTemplateImages = template.getDiskList();
Line 279:         String templateMeta = generateVmTemplateMetadata(template, 
allTemplateImages);
Line 280:         metaDictionary.put(template.getId(), new 
KeyValuePairCompat<String, List<Guid>>(
Line 281:                 templateMeta, LinqUtils.foreach(allTemplateImages, 
new Function<DiskImage, Guid>() {
We need to create a list of guids only - so having it with a for will make this 
code longer. that code was only moved here, i prefer to leave it like that in 
this patch.
Line 282:                     @Override
Line 283:                     public Guid eval(DiskImage diskImage) {
Line 284:                         return diskImage.getId().getValue();
Line 285:                     }


Line 301:      * loads additional need vm data for it's ovf
Line 302:      * @param vm
Line 303:      */
Line 304:     protected void loadVmData(VM vm) {
Line 305:         if (vm.getInterfaces().isEmpty()) {
That code was only moved here, i prefer to leave it like that in this patch as 
it's called also from export command - it can be handled in a further patch, i 
want to limit the scope of this patch to the addon of the ovf updater, not to 
handle the exist ovf build logic.
Line 306:             
vm.setInterfaces(getVmNetworkInterfaceDao().getAllForVm(vm.getId()));
Line 307:         }
Line 308:         if (StringUtils.isEmpty(vm.getVmtName())) {
Line 309:             if (!Guid.Empty.equals(vm.getVmtGuid())) {


....................................................
File 
backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/OvfDataUpdaterTest.java
Line 264:                         StoragePoolStatus.Maintanance.getValue());
Line 265:         pools.add(pool1);
Line 266:         pools.add(pool2);
Line 267:         pools.add(pool3);
Line 268:         return pools;
I prefer to have it like that as i want to have different pool statuses, what 
will be added is verification that update methods weren't called with the pool 
id which is in maintanance.
Line 269:     }
Line 270: 
Line 271:     private VM createVm(Guid id, VMStatus status) {
Line 272:         VM vm = new VM();


....................................................
File 
backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/vdscommands/UpdateVMVDSCommandParameters.java
Line 4: 
Line 5: import java.util.List;
Line 6: import java.util.HashMap;
Line 7: import java.util.Map;
Line 8: 
ctor now gets a Map instead of HashMap, therefore privateInfoDictionary is now 
declared as a Map
Line 9: public class UpdateVMVDSCommandParameters extends 
StorageDomainIdParametersBase {
Line 10:     public UpdateVMVDSCommandParameters(Guid storagePoolId,
Line 11:             Map<Guid, KeyValuePairCompat<String, List<Guid>>> 
infoDictionary) {
Line 12:         super(storagePoolId);


....................................................
File 
backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dao/StoragePoolDAODbFacadeImpl.java
Line 192:                 return entity;
Line 193:             }
Line 194:         };
Line 195: 
Line 196:         return getCallsHandler().executeReadList("GetAllByStatus", 
mapper, parameterSource);
I prefer to not have this patch dependent in other patch, i decided to add a 
mapper for this method and will make use of it in other methods as well in a 
further patch.
Line 197:     }
Line 198: 
Line 199:     @Override
Line 200:     public List<storage_pool> getAll(Guid userID, boolean isFiltered) 
{


--
To view, visit http://gerrit.ovirt.org/9328
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I9b5132300fb1f1fd94f771cab15efe5246dbeca8
Gerrit-PatchSet: 18
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Liron Aravot <[email protected]>
Gerrit-Reviewer: Allon Mureinik <[email protected]>
Gerrit-Reviewer: Arik Hadas <[email protected]>
Gerrit-Reviewer: Ayal Baron <[email protected]>
Gerrit-Reviewer: Liron Aravot <[email protected]>
Gerrit-Reviewer: Maor Lipchuk <[email protected]>
Gerrit-Reviewer: Michael Kublin <[email protected]>
Gerrit-Reviewer: Tal Nisan <[email protected]>
Gerrit-Reviewer: Vered Volansky <[email protected]>
Gerrit-Reviewer: Yair Zaslavsky <[email protected]>
Gerrit-Reviewer: liron aravot <[email protected]>
_______________________________________________
Engine-patches mailing list
[email protected]
http://lists.ovirt.org/mailman/listinfo/engine-patches

Reply via email to