Mike Kolesnik has posted comments on this change.
Change subject: DO NOT SUBMIT core: Introducing AddVmFromSnapshot command
......................................................................
Patch Set 21: (14 inline comments)
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/AddVmFromSnapshotCommand.java
Line 41: @LockIdNameAttribute(fieldName = "sourceSnapshotId")
Why add this annotation?
Line 76: getVm().setvmt_guid(VmTemplateHandler.BlankVmTemplateId);
Is this really needed here or should it be in the super class? (Seiing as the
super class always collapses)
Line 110: lockEntities();
Need to unlock if no tasks were created..
Line 124: continue;
Perhaps better instead of continue to use else, which will be clearer.
Line 189: diskImage.setimage_group_id(Guid.NewGuid());
Shouldn't you set also it_guid, parent id and such to empty or whatnot?
Otherwise you create a dependency for source images in the DB.
Line 231: protected boolean canDoAction() {
If you not chaining checks, you can just return the false immediately and no
need for the variable and code will be clearer..
Or as an alternative you can chain the commands as:
returnValue = validate1() && validate2() && etc...
Line 238: vmFromConfiguration = getVmFromConfiguration();
What if it is null?
Line 244: returnValue = super.canDoAction(); // Run all checks for
AddVm, now that it is determined snapshot exists
Please put the comment before the line, not inline
Line 263: private boolean checkVmHasDisks() {
I think it's simpler if you drop the check from the method name..
Line 267: result = false;
You can simply return false here, and return true at the end.
Line 273: ValidationResult validationResult = new
SnapshotsValidator().vmNotDuringSnapshot(getSnapshot().getVmId());
This variable can be inlined
Line 282: for (DiskImage img :
getParameters().getDiskInfoMap().values()) {
Only on the disks from parameters?
Also, what about illegal images?
Line 313: getVmDynamicDao().updateStatus(getVm().getId(),
VMStatus.ImageLocked);
I think you need to do it via lockVmWithCompensationIfNeeded() method.
Line 332: // TODO: Should be changed when multiple storage domain
will be implemented and the desired quotas will be
Wouldn't it be better (specifically in this case) to extract the 2 methods
getQuotaIdFromSourc
eVmEntity and getImagesForQuotaValidation in the parent command (AddVmCommand)
and override them there, instead of copy paste the entire code?
--
To view, visit http://gerrit.ovirt.org/2300
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I0ac7e0c801faf6bca9932c5ed47dc6a065c1ac51
Gerrit-PatchSet: 21
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Yair Zaslavsky <[email protected]>
Gerrit-Reviewer: Mike Kolesnik <[email protected]>
Gerrit-Reviewer: Yair Zaslavsky <[email protected]>
_______________________________________________
Engine-patches mailing list
[email protected]
http://lists.ovirt.org/mailman/listinfo/engine-patches
