Michael Kublin has posted comments on this change.
Change subject: core: introducing OvfAutoUpdate
......................................................................
Patch Set 14: I would prefer that you didn't submit this
(10 inline comments)
....................................................
File backend/manager/dbscripts/vms_sp.sql
Line 462:
Line 463:
Line 464:
Line 465:
Line 466:
These is the same as ta line 492?
Line 467: Create or replace FUNCTION
IncrementDbGenerationForAllInStoragePool(v_storage_pool_id UUID)
Line 468: RETURNS VOID
Line 469: AS $procedure$
Line 470: DECLARE
Line 466:
Line 467: Create or replace FUNCTION
IncrementDbGenerationForAllInStoragePool(v_storage_pool_id UUID)
Line 468: RETURNS VOID
Line 469: AS $procedure$
Line 470: DECLARE
why need to use a cursor, why not to use a JOIN ?
Line 471: curs CURSOR FOR SELECT vms.vm_guid FROM
Line 472: vm_static vms WHERE vms.vds_group_id IN (SELECT v.vds_group_id
FROM vds_groups v, storage_pool sp WHERE sp.id=v_storage_pool_id AND
v.storage_pool_id = sp.id)
Line 473: ORDER BY vm_guid;
Line 474: id UUID;
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/OvfDataUpdater.java
Line 115: */
Line 116: protected void updateOvfForVmsOfStoragePool(Guid poolId) {
Line 117: // get vm ids that need to be updated.
Line 118: List<Guid> vmsIdsForUpdate =
getVmAndTemplatesGenerationsDao().getVmsIdsForOvfUpdate(poolId);
Line 119: int i = 0;
these can be changed to for
Line 120: while (i < vmsIdsForUpdate.size()) {
Line 121: int size = Math.min(ITEMS_COUNT_PER_UPDATE,
vmsIdsForUpdate.size() - i);
Line 122: List<Guid> idsToProcess = vmsIdsForUpdate.subList(i,
i+size);
Line 123: i+= size;
Line 128: }
Line 129: }
Line 130: }
Line 131:
Line 132: private void clearProccessedInfo() {
use proccessedIdsInfo = new ...; It is faster and let to JVM make a work, I
think it knows about memory management more than you.
Line 133: proccessedIdsInfo.clear();
Line 134: proccessedOvfGenerationsInfo.clear();
Line 135: }
Line 136:
Line 133: proccessedIdsInfo.clear();
Line 134: proccessedOvfGenerationsInfo.clear();
Line 135: }
Line 136:
Line 137: private void removeOvfForTemplatesAndVmsOfStoragePool(Guid
poolId) {
Why create LinkedList?
Line 138: List<Guid> idsForRemoval = new
LinkedList<Guid>(getVmAndTemplatesGenerationsDao().getIdsForOvfDeletion(poolId));
Line 139:
Line 140: for (Guid id : idsForRemoval){
Line 141: executeRemoveVmInSpm(poolId, id, Guid.Empty);
Line 145: }
Line 146:
Line 147:
Line 148: protected void performOvfUpdate(Guid poolId , Map<Guid,
KeyValuePairCompat<String, List<Guid>>> vmsAndTemplateMetadata) {
Line 149: // update ovf metadata in vdsm
No need comments here, use javadoc on methods
Line 150: executeUpdateVmInSpmCommand(poolId, vmsAndTemplateMetadata,
Guid.Empty);
Line 151: // update vm/templates ovf generation to the db generation
that was updated in the storage.
Line 152: int i = 0;
Line 153: while (i<proccessedIdsInfo.size()) {
Line 175: VmTemplateHandler.UpdateDisksFromDb(template);
Line 176: boolean verifyDisksNotLocked =
verifyDisksNotLocked(template.getDiskList());
Line 177: if (verifyDisksNotLocked) {
Line 178: loadTemplateData(template);
Line 179: Long currentDbGeneration =
getVmStaticDao().getDbGeneration(template.getId());
autobixing?
Line 180: if (template.getDb_generation() ==
currentDbGeneration.longValue()) {
Line 181: buildMetadataDictionaryForTemplate(template,
vmsAndTemplateMetadata);
Line 182: proccessedIdsInfo.add(template.getId());
Line 183:
proccessedOvfGenerationsInfo.add(template.getDb_generation());
Line 196: */
Line 197: protected void updateOvfForTemplatesOfStoragePool(Guid poolId) {
Line 198: List<Guid> templateIdsForUpdate =
getVmAndTemplatesGenerationsDao().
Line 199: getVmTemplatesIdsForOvfUpdate(poolId);
Line 200: int i = 0;
These can be changed to for also
Line 201: while (i < templateIdsForUpdate.size()) {
Line 202: int size = Math.min(templateIdsForUpdate.size() - i,
ITEMS_COUNT_PER_UPDATE);
Line 203: List<Guid> idsToProcess = templateIdsForUpdate.subList(i,
i + size);
Line 204: i += size;
Line 224: if (VMStatus.ImageLocked != vm.getstatus()) {
Line 225: VmHandler.updateDisksFromDb(vm);
Line 226: if (verifyDisksNotLocked(vm.getDiskList())) {
Line 227: loadVmData(vm);
Line 228: Long currentDbGeneration =
getVmStaticDao().getDbGeneration(vm.getId());
autoboxing? long == Long in java
Line 229: if (vm.getStaticData().getDb_generation() ==
currentDbGeneration.longValue()) {
Line 230: buildMetadataDictionaryForVm(vm,
vmsAndTemplateMetadata);
Line 231: proccessedIdsInfo.add(vm.getId());
Line 232:
proccessedOvfGenerationsInfo.add(vm.getStaticData().getDb_generation());
Line 335: tempVar.setStorageDomainId(storageDomainId);
Line 336: return
Backend.getInstance().getResourceManager().RunVdsCommand(VDSCommandType.UpdateVM,
tempVar)
Line 337: .getSucceeded();
Line 338: }
Line 339:
I already said that these code is wrong, at least 3 times.
Line 340: protected boolean executeRemoveVmInSpm(Guid storagePoolId, Guid
vmId, Guid storageDomainId) {
Line 341: return
Backend.getInstance().getResourceManager().RunVdsCommand(VDSCommandType.RemoveVM,
Line 342: new RemoveVMVDSCommandParameters(storagePoolId, vmId,
storageDomainId)).getSucceeded();
Line 343: }
--
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: 14
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
