Ayal Baron has posted comments on this change.

Change subject: core: export/import of diskless VM/VM with no snappable 
disks(#838937)
......................................................................


Patch Set 3: I would prefer that you didn't submit this

(6 inline comments)

....................................................
File 
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/ExportVmCommand.java
Line 183:         getDisksBasedOnImage();
shouldn't this be 'getSnappableDisks' or something? (internal implementation 
could be getDisksBasedOnImage)

Line 421:             log.warn("ExportVmCommand::EndMoveVmCommand: Vm is null - 
not performing full EndAction");
what is the new behaviour now that you don't setSucceeded to true ?

Line 427:         EndActionOnAllImageGroups();
previously this was run even if vm == null.  Is there a situation where we'd 
have disks to end action on but vm == null? (sounds odd, but need to make sure 
why code worked as it did).

Line 452:             this.endDefaultOps(vm);
although I have no problem with it, just want to make sure that you realize 
that this op goes to vdsm and user is waiting in sync mode.

....................................................
File 
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/ImportVmCommand.java
Line 904:         if (getVm() != null) {
brrrr.
If what you want is to make sure you reach the db then create a method 
getVmFromDB which does:
setVm(null)
return getVm()

Right now it looks horrible.

Line 907:             UpdateVmImSpm();
If you get the chance, please send a patch to change this name to "In" and not 
"Im"

--
To view, visit http://gerrit.ovirt.org/6379
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I77565ffd66134b44d15f66cfdfa97422e3c11fb2
Gerrit-PatchSet: 3
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Liron Aravot <lara...@redhat.com>
Gerrit-Reviewer: Allon Mureinik <amure...@redhat.com>
Gerrit-Reviewer: Ayal Baron <aba...@redhat.com>
Gerrit-Reviewer: Daniel Erez <de...@redhat.com>
Gerrit-Reviewer: Liron Aravot <lara...@redhat.com>
Gerrit-Reviewer: Maor Lipchuk <mlipc...@redhat.com>
Gerrit-Reviewer: Michael Kublin <mkub...@redhat.com>
Gerrit-Reviewer: Tal Nisan <tni...@redhat.com>
_______________________________________________
Engine-patches mailing list
Engine-patches@ovirt.org
http://lists.ovirt.org/mailman/listinfo/engine-patches

Reply via email to