Allon Mureinik has posted comments on this change.

Change subject: core: OvfDataUpdater - removal calls to updateVm/TemplateInSpm
......................................................................


Patch Set 16: I would prefer that you didn't submit this

(12 inline comments)

....................................................
File 
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/AddVmFromScratchCommand.java
Line 114: 
Line 115:             
VmHandler.LockVm(getParameters().getVmStaticData().getId());
Line 116:         } else {
Line 117:             // if no disks send update vm here
Line 118:             getVmStaticDao().incrementDbGeneration(getVm().getId());
shouldn't you also increment the DB version if there /ARE/ disks in the VM?
Line 119:         }
Line 120: 
Line 121:         return ret;
Line 122:     }


....................................................
File 
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/ExportVmCommand.java
Line 407:         
OvfDataUpdater.getInstance().buildMetadataDictionaryForVm(getVm(), 
metaDictionary);
Line 408:         VmHandler.updateDisksFromDb(getVm());
Line 409:         return 
OvfDataUpdater.getInstance().executeUpdateVmInSpmCommand(getVm().getstorage_pool_id(),
Line 410:                 metaDictionary,
Line 411:                 getParameters().getStorageDomainId());
can you explain this change?
Line 412:     }
Line 413: 
Line 414:     @Override
Line 415:     protected void endSuccessfully() {


....................................................
File 
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/ExportVmTemplateCommand.java
Line 141:         
OvfDataUpdater.getInstance().buildMetadataDictionaryForTemplate(getVmTemplate(),
 metaDictionary);
Line 142:         
OvfDataUpdater.getInstance().executeUpdateVmInSpmCommand(getVmTemplate().getstorage_pool_id().getValue(),
Line 143:                 metaDictionary,
Line 144:                 getParameters().getStorageDomainId());
Line 145:     }
this one too?
Line 146: 
Line 147:     @Override
Line 148:     public Map<String, String> getJobMessageProperties() {
Line 149:         if (jobProperties == null) {


....................................................
File 
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/MoveOrCopyTemplateCommand.java
Line 256:             
log.warn("MoveOrCopyTemplateCommand::EndMoveOrCopyCommand: VmTemplate is null, 
not performing full EndAction");
Line 257:         }
Line 258:     }
Line 259: 
Line 260:     protected void updateTemplateInSpm() {
please rename the method, as it no longer updates the OVF in the SPM.
Line 261:         VmTemplate vmt = getVmTemplate();
Line 262:         getVmStaticDAO().incrementDbGeneration(vmt.getId());
Line 263:     }
Line 264: 


....................................................
File 
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/MoveVmCommand.java
Line 176:         setSucceeded(true);
Line 177: 
Line 178:     }
Line 179: 
Line 180:     protected void updateVmImSpm() {
please rename the method, as it changed it's function
Line 181:         getVmStaticDAO().incrementDbGeneration(getVm().getId());
Line 182:     }
Line 183: 
Line 184:     @Override


....................................................
File 
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/TryBackToAllSnapshotsOfVmCommand.java
Line 80:     }
Line 81: 
Line 82:     @Override
Line 83:     protected void endSuccessfully() {
Line 84:         getVmStaticDAO().incrementDbGeneration(getVm().getId());
I assume the entire method is run in a single transaction, right?
Line 85:         endActionOnDisks();
Line 86: 
Line 87:         if (getVm() != null) {
Line 88:             VmHandler.unlockVm(getVm(), getCompensationContext());


....................................................
File backend/manager/modules/dal/src/main/jdbc-resources/engine-daos.properties
Line 54: 
GlusterVolumeDao=org.ovirt.engine.core.dao.gluster.GlusterVolumeDaoDbFacadeImpl
Line 55: 
GlusterBrickDao=org.ovirt.engine.core.dao.gluster.GlusterBrickDaoDbFacadeImpl
Line 56: 
GlusterOptionDao=org.ovirt.engine.core.dao.gluster.GlusterOptionDaoDbFacadeImpl
Line 57: 
ImageStorageDomainMapDao=org.ovirt.engine.core.dao.ImageStorageDomainMapDaoDbFacadeImpl
Line 58: 
VmAndTemplatesGenerationsDAO=org.ovirt.engine.core.dao.VmAndTemplatesGenerationsDbFacadeImpl
This should be a part of the previous patch.


....................................................
File 
backend/manager/modules/dal/src/test/java/org/ovirt/engine/core/dao/FixturesTool.java
Line 4: 
Line 5: /**
Line 6:  * A utility class for DAO testing which maps the fixtures entities to 
constants, for easy testing.
Line 7:  */
Line 8: public class FixturesTool {
All the change to this file should be a part of the previous patch.
Line 9:     /**
Line 10:      * Predefined NFS storage pool.
Line 11:      */
Line 12:     protected static final Guid STORAGE_POOL_NFS = new 
Guid("72b9e200-f48b-4687-83f2-62828f249a47");


....................................................
File 
backend/manager/modules/dal/src/test/java/org/ovirt/engine/core/dao/VmStaticDAOTest.java
Line 1: /**
Line 2:  *
All the change to this file should be a part of the previous patch.
Line 3:  */
Line 4: package org.ovirt.engine.core.dao;
Line 5: 
Line 6: import static org.junit.Assert.assertEquals;


....................................................
File backend/manager/modules/dal/src/test/resources/fixtures.xml
Line 847:             <value>2010-12-01 09:52:57</value>
Line 848:             <value>4</value>
Line 849:         </row>
Line 850:     </table>
Line 851:     
All the change to this file should be a part of the previous patch.
Line 852:     <table name="vm_ovf_generations">
Line 853:         <column>vm_guid</column>
Line 854:         <column>storage_pool_id</column>
Line 855:         <column>ovf_generation</column>


Line 896:       <row>
Line 897:               <value>77296e00-0cad-4e5a-9299-008a7b6f4360</value>
Line 898:               <value>72b9e200-f48b-4687-83f2-62828f249a47</value>
Line 899:               <value>1</value>
Line 900:       </row>
replace tabs with spaces
Line 901:     </table>
Line 902: 
Line 903:     <table name="vm_static">
Line 904:         <column>vm_guid</column>


....................................................
Commit Message
Line 3: AuthorDate: 2012-12-02 09:59:28 +0200
Line 4: Commit:     Liron Aravot <lara...@redhat.com>
Line 5: CommitDate: 2012-12-02 13:38:26 +0200
Line 6: 
Line 7: core: OvfDataUpdater - removal calls to updateVm/TemplateInSpm
s/removal/remove/

Also, missing an explanation why and how this is done.
Line 8: 
Line 9: Change-Id: Iedebceb9809dc0b11c0bbe8a2d4af63b0d848df1


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Iedebceb9809dc0b11c0bbe8a2d4af63b0d848df1
Gerrit-PatchSet: 16
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Liron Aravot <lara...@redhat.com>
Gerrit-Reviewer: Allon Mureinik <amure...@redhat.com>
Gerrit-Reviewer: Ayal Baron <aba...@redhat.com>
Gerrit-Reviewer: Daniel Erez <de...@redhat.com>
Gerrit-Reviewer: Liron Aravot <lara...@redhat.com>
Gerrit-Reviewer: Maor Lipchuk <mlipc...@redhat.com>
Gerrit-Reviewer: Michael Kublin <mkub...@redhat.com>
Gerrit-Reviewer: Vered Volansky <vvola...@redhat.com>
Gerrit-Reviewer: Yair Zaslavsky <yzasl...@redhat.com>
_______________________________________________
Engine-patches mailing list
Engine-patches@ovirt.org
http://lists.ovirt.org/mailman/listinfo/engine-patches

Reply via email to