Yair Zaslavsky has posted comments on this change.
Change subject: engine-core: Multiple storage domains when importing VM
......................................................................
Patch Set 1: (9 inline comments)
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/AddVmFromTemplateCommand.java
Line 18: //TODO: revisit - template disks may come from different storage
domains
Have you revisited this? if yes - remove the comment.
If you did - that's fine and ignore, I'm still at the first file, of the first
patch:)
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/ImportVmCommand.java
Line 58: public class ImportVmCommand extends
MoveOrCopyTemplateCommand<ImportVmParameters> {
Not sure I understand why you wanted to fix the definition of the class, and
not have it as Generic any more.
Line 161: if(retVal) {
Question (maybe some sort of a hunch) - I have a feeling this check you added
might be used in other places (i.e: the feature I'm working on) - any plans on
maybe move it to a utils class?
Line 275:
addCanDoActionMessage(VdcBllMessages.ACTION_TYPE_FAILED_DISK_SPACE_LOW);
Do we want to indicate somehow which storage domain is problematic and fails
our can do action?
Line 437: Map<Guid, Guid> imageDomainMap =
getParameters().getImageMap();
General question - can you define a field in the class for this map, and at the
CTOR fill its value? It will be easier for reuse (not having to code
getParameters().getImageMap() all the time).
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/ImportVmTemplateCommand.java
Line 68: Map<Guid, Guid> destinationStorageDomainMap =
getParameters().getImageMap();
Already asked about the field for image map before, just a reminder.
Line 179: if(!retVal) {
reminder - might be nice to know the problematic storage domain
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/MoveOrCopyTemplateCommand.java
Line 277: protected boolean domainIsValidDestination(storage_domains
domain) {
I really suspect this might be relevant not just for templates. Maybe move it
to some helper class and have a static method?
Line 286: protected Map<storage_domains, Integer>
getSpaceRequirementsForStorageDomains(List<DiskImage> images) {
Same as above
--
To view, visit http://gerrit.ovirt.org/1067
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I67ebc1b402194897cf2ed12f45228153e2efe153
Gerrit-PatchSet: 1
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Jonathan Choate <[email protected]>
Gerrit-Reviewer: Michael Kublin <[email protected]>
Gerrit-Reviewer: Mike Kolesnik <[email protected]>
Gerrit-Reviewer: Omer Frenkel <[email protected]>
Gerrit-Reviewer: Yair Zaslavsky <[email protected]>
_______________________________________________
Engine-patches mailing list
[email protected]
http://lists.ovirt.org/mailman/listinfo/engine-patches
