Mike Kolesnik has posted comments on this change.
Change subject: core: DO NOT SUBMIT BLL: Using DiskImage instead of DiskImage
template
......................................................................
Patch Set 7: I would prefer that you didn't submit this
(31 inline comments)
Please see individual comments
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/AddVmCommand.java
Line 207
Why delete the newline?
Line 230
Why delete the newline?
Line 504:
Why all these additional newlines? there should be only one
Line 521
Why delete the newline?
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/AddVmFromScratchCommand.java
Line 102: return (disks.size()>0) ?
ConcreteAddVmImages(disks.get(0).getId()): true;
Why not check isEmpty?
Also please format this line
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/AddVmFromTemplateCommand.java
Line 72: } }
Why delete the newline?
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/BaseImagesCommand.java
Line 84: protected DiskImage getDiskImage() {
Why add commented out code?
Line 108: _diskImage = (tmpImage == null ||
tmpImage.getVmEntityType() == VmEntityType.TEMPLATE)?(null):(tmpImage);
I'm not sure this logic is correct, since DiskImage is linked to template
either way and is in fact a DiskImage instance.
Either way, since you don't use IImage anymore then this call should probably
be removed and the duplicate field (_diskImage) with it..
Line 242
Why did you delete the not?
It is there so that you get the images that are not the one in the command,
this way it will break snapshots code..
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/CreateImageTemplateCommand.java
Line 61: newImage.setVmEntityType(VmEntityType.TEMPLATE);
Why set this? This is not saved by this DAO..
Line 84: image_vm_map imageVmMap = new
image_vm_map(true,newImage.getId(),getImageContainerId());
Please format this line.
Line 88: // 1. as vtmid_guid
No need for these comments anymore
Line 101
Why delete the newline?
Line 140
You should remove the image_vm_map instead
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/CreateSnapshotFromTemplateCommand.java
Line 36: .getDiskImageDAO().get(getImageId());
In this case, isn't this the same as super.getImage()?
Why override and have local field for that?
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/GetVmTemplatesDisksQuery.java
Line 21: return
DbFacade.getInstance().getDiskImageDAO().getAllForVm(getParameters().getId());
In this case you can ditch the method and do the call directly from execute
Line 32
Why delete the newline?
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/ImportVmCommand.java
Line 63: private Map<Guid,Guid> imageToDestinationDomainMap = new
HashMap<Guid, Guid>();
No need to initialize at all since its initialized in ctor
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/ImportVmTemplateCommand.java
Line 50: public Map<Guid,Guid> imageToDestinationDomainMap = new
HashMap<Guid, Guid>();
No need to initialize at all since it's initialized in ctor
Line 320: image_vm_map imageVmMap = new
image_vm_map(image.getactive(),image.getId(),image.getvm_guid());
Please format this line
Line 338
Why delete the newline?
Line 384
Shouldn't you delete the image_vm_map also?
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/MoveOrCopyImageGroupCommand.java
Line 54: Collection<DiskImage> templateDisks =
getVmTemplate().getDiskMap().values();
Instead, it would be better to query DiskImageDao.getAllSnapshotsForImageGroup
and get the first (or null if the result is empty).
You can also apply this logic to the vm case (above).
This will save time since DB call gets only the correct image, and also saves
the loop, and the code is more readable.
Line 72: protected DiskImage getDiskImage() {
Why is this needed?
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/RemoveAllVmTemplateImageTemplatesCommand.java
Line 84: image_vm_map_id imageVmMap = new
image_vm_map_id(diskImage.getId(),diskImage.getvm_guid());
You can inline this variable
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/RemoveTemplateSnapshotCommand.java
Line 61: protected DiskImage getDiskImage() {
Why is this needed?
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/storage/GetStorageDomainsByVmTemplateIdQuery.java
Line 58:
Why add newline?
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/VmTemplateCommand.java
Line 92: List<DiskImage> vmtImages = DbFacade.getInstance()
No need for newline here.
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/VmTemplateHandler.java
Line 48: vmt.getDiskList().add(dit);
You can use addAll, or better yet just assign the disk list to the template.
Line 67: public static java.util.ArrayList<DiskImage>
GetDiskImageListByDiskImageTemplateList(VmTemplate vmTemplate,
This method is unused and can be deleted
....................................................
File
backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/itests/BasicTestSetup.java
Line 201: "",VmEntityType.VM);
Please add newline before the parameter
--
To view, visit http://gerrit.ovirt.org/1570
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I701bc85d005e3b1636feb8cb6cedca099356614c
Gerrit-PatchSet: 7
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Yair Zaslavsky <yzasl...@redhat.com>
Gerrit-Reviewer: Mike Kolesnik <mkole...@redhat.com>
Gerrit-Reviewer: Omer Frenkel <ofren...@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
