Gilad Chaplik has posted comments on this change. Change subject: core, db: disk profile for disk image ......................................................................
Patch Set 13: (9 comments) new patch to follow http://gerrit.ovirt.org/#/c/29036/13/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/AddDiskCommand.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/AddDiskCommand.java: Line 567: } Line 568: Line 569: private Guid getDiskProfileId() { Line 570: if (getParameters().getDiskInfo() != null Line 571: && DiskStorageType.IMAGE == getParameters().getDiskInfo().getDiskStorageType()) { > the if can be removed (and so the method) Done Line 572: return getDiskImageInfo().getDiskProfileId(); Line 573: } Line 574: return null; Line 575: } http://gerrit.ovirt.org/#/c/29036/13/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/AddVmCommand.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/AddVmCommand.java: Line 704: ArrayList<Guid> storageIds = new ArrayList<Guid>(); Line 705: storageIds.add(storageId); Line 706: newImage.setStorageIds(storageIds); Line 707: newImage.setQuotaId(image.getQuotaId()); Line 708: newImage.setDiskProfileId(image.getDiskProfileId()); > are you sure the the profile is always of the given domain? since it's similar to quota, I'm willing to bet... Line 709: return newImage; Line 710: } Line 711: Line 712: protected boolean canAddVm(List<String> reasons, String name, Guid storagePoolId, http://gerrit.ovirt.org/#/c/29036/13/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/BaseImagesCommand.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/BaseImagesCommand.java: Line 265: diskDynamic.setId(image.getImageId()); Line 266: diskDynamic.setactual_size(image.getActualSizeInBytes()); Line 267: getDiskImageDynamicDAO().save(diskDynamic); Line 268: image_storage_domain_map image_storage_domain_map = new image_storage_domain_map(image.getImageId(), Line 269: image.getStorageIds().get(0), image.getQuotaId(), image.getDiskProfileIds().get(0)); > why not .getDiskProfileId? Done Line 270: getImageStorageDomainMapDao().save(image_storage_domain_map); Line 271: boolean isDiskAdded = saveDiskIfNotExists(image); Line 272: if (compensationContext != null) { Line 273: compensationContext.snapshotNewEntity(image.getImage()); Line 401: static public image_storage_domain_map saveImage(DiskImage diskImage) { Line 402: DbFacade.getInstance().getImageDao().save(diskImage.getImage()); Line 403: image_storage_domain_map image_storage_domain_map = new image_storage_domain_map(diskImage.getImageId(), Line 404: diskImage.getStorageIds() Line 405: .get(0), diskImage.getQuotaId(), diskImage.getDiskProfileIds().get(0)); > same Done Line 406: DbFacade.getInstance() Line 407: .getImageStorageDomainMapDao() Line 408: .save(image_storage_domain_map); Line 409: return image_storage_domain_map; http://gerrit.ovirt.org/#/c/29036/13/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/TryBackToAllSnapshotsOfVmCommand.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/TryBackToAllSnapshotsOfVmCommand.java: Line 208: params.setVmSnapshotId(newActiveSnapshotId); Line 209: params.setEntityInfo(getParameters().getEntityInfo()); Line 210: params.setParentParameters(getParameters()); Line 211: params.setQuotaId(image.getQuotaId()); Line 212: params.setDiskProfileId(image.getDiskProfileId()); > why do we need a profile id when we do a preview of snapshot? Done Line 213: return params; Line 214: } Line 215: }); Line 216: } http://gerrit.ovirt.org/#/c/29036/13/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/UpdateVmDiskCommand.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/UpdateVmDiskCommand.java: Line 382: Line 383: protected void updateDiskProfile(DiskImage diskImage) { Line 384: if (isDiskImage()) { Line 385: DiskImage oldDisk = (DiskImage) getOldDisk(); Line 386: if (!Objects.equals(oldDisk.getDiskProfileId(), diskImage.getDiskProfileId())) { > it seems like the profile will never be updated.. as in applyUserChanges() this code should stay, and applyUserChanges should be removed.. Line 387: getImageStorageDomainMapDao().updateDiskProfileByImageGroupId(diskImage.getId(), Line 388: diskImage.getStorageIds().get(0), Line 389: diskImage.getDiskProfileId()); Line 390: } http://gerrit.ovirt.org/#/c/29036/13/backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/businessentities/DiskImage.java File backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/businessentities/DiskImage.java: Line 534: && ObjectUtils.objectsEqual(readLatency, other.readLatency) Line 535: && ObjectUtils.objectsEqual(writeLatency, other.writeLatency) Line 536: && ObjectUtils.objectsEqual(flushLatency, other.flushLatency) Line 537: && ObjectUtils.objectsEqual(diskProfileIds, other.diskProfileIds) Line 538: && ObjectUtils.objectsEqual(diskProfileNames, other.diskProfileNames)); > we'll get true here only if the lists has the same objects in the same orde iirc we can.. lets leave it for now, in rare cases we'll have 2 DP Line 539: } Line 540: http://gerrit.ovirt.org/#/c/29036/13/backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dao/ImageStorageDomainMapDao.java File backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dao/ImageStorageDomainMapDao.java: Line 53: * @param diskId Line 54: * @param storageDomainId Line 55: * @param diskProfileId Line 56: */ Line 57: void updateDiskProfileByImageGroupId(Guid diskId, Guid storageDomainId, Guid diskProfileId); > i'd add here "AndStorageDomainId" actually it's just diskProfileId. http://gerrit.ovirt.org/#/c/29036/13/backend/manager/modules/dal/src/test/java/org/ovirt/engine/core/dao/ImageStorageDomainMapDaoTest.java File backend/manager/modules/dal/src/test/java/org/ovirt/engine/core/dao/ImageStorageDomainMapDaoTest.java: Line 107: } Line 108: Line 109: @Test Line 110: public void testChangeDiskProfileForDisk() { Line 111: // fetch image > I'd test with few images per disk. will try to add on more tests in future. Line 112: image_storage_domain_map image_storage_domain_map = dao.getAllByImageId(EXISTING_IMAGE_ID).get(0); Line 113: // test that the current disk profile doesn't equal with the new disk profile Line 114: assertThat("Same source and dest disk profile id, cannot perform test", Line 115: image_storage_domain_map.getDiskProfileId(), not(equalTo(FixturesTool.DISK_PROFILE_2))); -- To view, visit http://gerrit.ovirt.org/29036 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I9488a1e31e85013a01ee088fdf71854f765059c9 Gerrit-PatchSet: 13 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Gilad Chaplik <gchap...@redhat.com> Gerrit-Reviewer: Allon Mureinik <amure...@redhat.com> Gerrit-Reviewer: Arik Hadas <aha...@redhat.com> Gerrit-Reviewer: Daniel Erez <de...@redhat.com> Gerrit-Reviewer: Doron Fediuck <dfedi...@redhat.com> Gerrit-Reviewer: Gilad Chaplik <gchap...@redhat.com> Gerrit-Reviewer: Liron Aravot <lara...@redhat.com> Gerrit-Reviewer: Tal Nisan <tni...@redhat.com> Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes _______________________________________________ Engine-patches mailing list Engine-patches@ovirt.org http://lists.ovirt.org/mailman/listinfo/engine-patches
