Allon Mureinik has posted comments on this change.
Change subject: engine: Move quota to Image_storage_domain_map
......................................................................
Patch Set 2: Code-Review-1
(22 comments)
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/AddVmCommand.java
Line 48: import org.ovirt.engine.core.common.businessentities.VmType;
Line 49: import org.ovirt.engine.core.common.businessentities.VmWatchdog;
Line 50: import org.ovirt.engine.core.common.businessentities.permissions;
Line 51: import
org.ovirt.engine.core.common.businessentities.network.VmInterfaceType;
Line 52: import org.ovirt.engine.core.common.businessentities.network.VmNic;
All of the changes here are unrelated to the patch - please remove them.
Line 53: import org.ovirt.engine.core.common.config.Config;
Line 54: import org.ovirt.engine.core.common.config.ConfigValues;
Line 55: import org.ovirt.engine.core.common.errors.VdcBLLException;
Line 56: import org.ovirt.engine.core.common.errors.VdcBllErrors;
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/ChangeQuotaForDiskCommand.java
Line 71
Line 72
Line 73
Line 74
Line 75
why is removing this getter relevant to the patch?
Line 47:
Line 48: @Override
Line 49: public List<QuotaConsumptionParameter>
getQuotaStorageConsumptionParameters() {
Line 50: List<QuotaConsumptionParameter> list = new
ArrayList<QuotaConsumptionParameter>();
Line 51: if (!getQuotaId().equals(disk.getQuotaId())) {
what if the quota id is null?
Line 52: if (disk.getQuotaId() != null &&
!Guid.Empty.equals(disk.getQuotaId())) {
Line 53: list.add(new QuotaStorageConsumptionParameter(
Line 54: disk.getQuotaId(),
Line 55: null,
Line 48: @Override
Line 49: public List<QuotaConsumptionParameter>
getQuotaStorageConsumptionParameters() {
Line 50: List<QuotaConsumptionParameter> list = new
ArrayList<QuotaConsumptionParameter>();
Line 51: if (!getQuotaId().equals(disk.getQuotaId())) {
Line 52: if (disk.getQuotaId() != null &&
!Guid.Empty.equals(disk.getQuotaId())) {
quota can no longer be empty - not sure if related to this patch.
Line 53: list.add(new QuotaStorageConsumptionParameter(
Line 54: disk.getQuotaId(),
Line 55: null,
Line 56: QuotaConsumptionParameter.QuotaAction.RELEASE,
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/ImagesHandler.java
Line 263: */
Line 264: public static void addDiskImageWithNoVmDevice(DiskImage image) {
Line 265: addDiskImageWithNoVmDevice(image,
Line 266: image.getActive(),
Line 267: new image_storage_domain_map(image.getImageId(),
image.getStorageIds().get(0), image.getQuotaId()));
wasn't the point of this patch removing the quota property of the image?
Line 268: }
Line 269:
Line 270: /**
Line 271: * Adds disk to a VM without creating a VmDevice entry
Line 287: * DiskImage to add
Line 288: */
Line 289: public static void addDiskImage(DiskImage image, Guid vmId) {
Line 290: addDiskImage(image, image.getActive(), new
image_storage_domain_map(image.getImageId(), image.getStorageIds()
Line 291: .get(0), image.getQuotaId()), vmId);
wasn't the point of this patch removing the quota property of the image?
Line 292: }
Line 293:
Line 294: /**
Line 295: * Add image and related entities to DB (Adds image, disk image
dynamic and image storage domain map)
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/RemoveAllVmTemplateImageTemplatesCommand.java
Line 67: vdcReturnValue.getFault().getMessage());
Line 68: }
Line 69:
Line 70:
DbFacade.getInstance().getImageStorageDomainMapDao().remove(
Line 71: new
ImageStorageDomainMapId(template.getImageId(), domain, template.getQuotaId()));
wasn't the point of this patch removing the quota property of the image?
Line 72: noImagesRemovedYet = false;
Line 73: }
Line 74:
Line 75: // remove images from db only if removing template
completely
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/UpdateVmDiskCommand.java
Line 311:
Line 312: protected void updateQuota(DiskImage diskImage) {
Line 313: if (isQuotaValidationNeeded()) {
Line 314: DiskImage oldDisk = (DiskImage) getOldDisk();
Line 315: if (!ObjectUtils.objectsEqual(oldDisk.getQuotaId(),
diskImage.getQuotaId())) {
wasn't the point of this patch removing the quota property of the image?
Line 316:
getImageStorageDomainMapDao().updateQuotaForImageAndSnapshots(diskImage.getId(),
Line 317: diskImage.getStorageIds().get(0),
Line 318: diskImage.getQuotaId());
Line 319: }
....................................................
File
backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/businessentities/DiskImage.java
Line 311: public void setQuotaIds(ArrayList<Guid> quotaIds) {
Line 312: this.quotaIds = quotaIds;
Line 313: }
Line 314:
Line 315: public Guid getQuotaId() {
Yikes...
Perhaps getFirstQuotaId()? Frankly. I'd remove this completely.
Line 316: if (quotaIds == null || quotaIds.isEmpty()) {
Line 317: return null;
Line 318: }
Line 319: return quotaIds.get(0);
Line 318: }
Line 319: return quotaIds.get(0);
Line 320: }
Line 321:
Line 322: public void setQuotaId(Guid quotaId) {
This too.
Line 323: quotaIds = new ArrayList<Guid>();
Line 324: quotaIds.add(quotaId);
Line 325: }
Line 326:
Line 336: if (quotaNames == null || quotaNames.isEmpty()) {
Line 337: return null;
Line 338: }
Line 339: return quotaNames.get(0);
Line 340: }
Same logic for these two
Line 341:
Line 342: public static DiskImage copyOf(DiskImage diskImage) {
Line 343: DiskImage di = new DiskImage();
Line 344:
....................................................
File
backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/businessentities/ImageStorageDomainMapId.java
Line 38: private Guid storageDomainId;
Line 39:
Line 40: private Guid imageId;
Line 41:
Line 42: private Guid quotaId;
This should not be here.
This class is only intended as a PK of the table, not as a data entity on its
own right.
Line 43:
Line 44: public Guid getStorageDomainId() {
Line 45: return storageDomainId;
Line 46: }
....................................................
File
backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/businessentities/image_storage_domain_map.java
Line 48: public int hashCode() {
Line 49: final int prime = 31;
Line 50: int result = 1;
Line 51: result = prime * result + ((id == null) ? 0 : id.hashCode());
Line 52: return result;
You added a member - it should reflect in the hashCode...
Line 53: }
Line 54:
Line 55: @Override
Line 56: public boolean equals(Object obj) {
Line 62: }
Line 63: if (!(obj instanceof image_storage_domain_map)) {
Line 64: return false;
Line 65: }
Line 66: image_storage_domain_map other = (image_storage_domain_map)
obj;
... and the equals.
Line 67: return (ObjectUtils.objectsEqual(id, other.id));
Line 68: }
Line 69:
Line 70: @Override
....................................................
File
backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dao/ImageStorageDomainMapDao.java
Line 44: * @param diskId
Line 45: * @param storageDomainId
Line 46: * @param quotaId
Line 47: */
Line 48: void updateQuotaForImageAndSnapshots(Guid diskId, Guid
storageDomainId, Guid quotaId);
why is this different from update(image_storage_domain_map)?
....................................................
File
backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dao/ImageStorageDomainMapDaoDbFacadeImpl.java
Line 73: MapSqlParameterSource parameterSource =
getCustomMapSqlParameterSource()
Line 74: .addValue("disk_id", diskId)
Line 75: .addValue("storage_domain_id", storageDomainId)
Line 76: .addValue("quota_id", quotaId);
Line 77:
getCallsHandler().executeModification("updateQuotaForImageAndSnapshots",
parameterSource);
see comment in the interface
Line 78: }
Line 79:
Line 80: private static final RowMapper<image_storage_domain_map>
IMAGE_STORAGE_DOMAIN_MAP_MAPPER =
Line 81: new RowMapper<image_storage_domain_map>() {
....................................................
File
backend/manager/modules/dal/src/test/java/org/ovirt/engine/core/dao/ImageStorageDomainMapDaoTest.java
Line 84: assertTrue(entries.isEmpty());
Line 85: }
Line 86:
Line 87: @Test
Line 88: public void testChangeQuotaForDisk() {
please fix the name and the comments.
Line 89: // fetch image
Line 90: image_storage_domain_map image_storage_domain_map =
dao.getAllByImageId(EXISTING_IMAGE_ID).get(0);
Line 91: Guid imageId = image_storage_domain_map.getimage_id();
Line 92: Guid quotaId = image_storage_domain_map.getQuotaId();
....................................................
File backend/manager/modules/dal/src/test/resources/fixtures.xml
Line 3874: </row>
Line 3875: <row>
Line 3876: <value>42058975-3d5e-484a-80c1-01c31207f579</value>
Line 3877: <value>72e3a666-89e1-4005-a7ca-f7548004a9ab</value>
Line 3878: <null/>
nice!
Line 3879: </row>
Line 3880: <row>
Line 3881: <value>42058975-3d5e-484a-80c1-01c31207f577</value>
Line 3882: <value>72e3a666-89e1-4005-a7ca-f7548004a9ab</value>
....................................................
File
backend/manager/modules/utils/src/main/java/org/ovirt/engine/core/utils/GuidUtils.java
Line 50: }
Line 51: for (String guidString : str.split(SEPARATOR)) {
Line 52: guidList.add(Guid.createGuidFromString(guidString));
Line 53: }
Line 54: return guidList;
seriously?
Line 55: }
....................................................
File packaging/dbscripts/create_views.sql
Line 114: array_to_string(array_agg(storage_domain_static.storage),
',') as storage_path,
Line 115: array_to_string(array_agg(storage_domain_static.id), ',')
storage_id,
Line 116:
array_to_string(array_agg(storage_domain_static.storage_name), ',') as
storage_name,
Line 117: array_to_string(array_agg(quota.id), ',') as quota_id,
Line 118: array_to_string(array_agg(quota.quota_name), ',') as
quota_name
I'd lose this - see comment on the business entities.
Line 119: FROM images
Line 120: LEFT JOIN image_storage_domain_map ON
image_storage_domain_map.image_id = images.image_guid
Line 121: LEFT OUTER JOIN storage_domain_static ON
image_storage_domain_map.storage_domain_id = storage_domain_static.id
Line 122: LEFT OUTER JOIN quota ON image_storage_domain_map.quota_id = quota.id
....................................................
File packaging/dbscripts/image_storage_domain_map_sp.sql
Line 71:
Line 72: END; $procedure$
Line 73: LANGUAGE plpgsql;
Line 74:
Line 75: Create or replace FUNCTION updateQuotaForImageAndSnapshots(v_disk_id
UUID, v_storage_domain_id UUID, v_quota_id UUID)
see comment on the DAO.
Line 76: RETURNS VOID
Line 77: AS $procedure$
Line 78: BEGIN
Line 79: UPDATE image_storage_domain_map as isdm
....................................................
File packaging/dbscripts/upgrade/03_03_0990_move_quota_id.sql
Line 1: -- add quota id column to the image-storage map table
Line 2: SELECT fn_db_add_column('image_storage_domain_map','quota_id', 'UUID
NULL');
Line 3: -- create a FK to quota table to that column
Line 4: ALTER TABLE image_storage_domain_map ADD CONSTRAINT
fk_image_storage_domain_map_quota FOREIGN KEY (quota_id) REFERENCES quota(id)
ON DELETE SET NULL;
I'd do this after the copying - should be faster.
Line 5: -- copy old quota from images table
Line 6: UPDATE image_storage_domain_map SET quota_id = (SELECT quota_id FROM
images WHERE image_id = image_guid);
Line 7: -- remove quota_id column from images table
--
To view, visit http://gerrit.ovirt.org/20097
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Icc81b077cf125de85fcb3586bc0d0f71e5e0716a
Gerrit-PatchSet: 2
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Gilad Chaplik <gchap...@redhat.com>
Gerrit-Reviewer: Allon Mureinik <amure...@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: oVirt Jenkins CI Server
Gerrit-HasComments: Yes
_______________________________________________
Engine-patches mailing list
Engine-patches@ovirt.org
http://lists.ovirt.org/mailman/listinfo/engine-patches
