Michael Kublin has posted comments on this change.
Change subject: engine-core: Move disk command
......................................................................
Patch Set 2: (12 inline comments)
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/MoveDiskCommand.java
Line 33: super(parameters);
I don't like the following constructor, during creation of class we are trying
to perform a queries to DB, if for some reason they will failed , the exception
will be something like that java.lang.ReflectionException or
InvocationException, which will not provide us with any clue for reason of
failure.
Line 37: targetDomainValidator = new
StorageDomainValidator(getParameters().getDestinationDomain(),
getParameters().getStoragePoolId());
These is also additional reason why I don't like an idea of DiskValidator or
way how it retrieves a disk, here u performed a query which was already done at
constructor of DiskValidator. Same also at VmValidator and possible for any
other. We paid performance for nothing
Line 47: vmIsDown() &&
vmIsDown() == true than vmIsUnlocked() == true
vmIsDown() == false than vmIsUnlocked() will not be checked.
Line 60: protected void executeCommand() {
Because of u checked lock status of VM, I think that's why here it is better to
use VmHandler.checkStatusAndLockVm()
Line 61: VmHandler.LockVm(getVm().getDynamicData(),
getCompensationContext());
You don't need to open transaction here, there are no reason for that
Line 100:
unneeded, explained before
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/transaction/MoveOrCopyImageGroupTransaction.java
Line 11:
You don't need such class.
If you want to run internal action with transaction u can set a parameter
setTransactionScopeOption , by the way a default scope is Required.
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/validator/DiskValidator.java
Line 8:
I don't see a reason for the following class, u want to use it at some other
places, if not it is unneeded. The same is regards any other validator
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/validator/StorageDomainValidator.java
Line 65: Map<storage_domains, Integer> map = new
HashMap<storage_domains, Integer>();
unneeded check : imageToDomainMap.isEmpty().
It is not , but at nay case
Line 68: storage_domains domain = entry.getValue();
bad casting, the size is double, we just lost information.
It is not , but at nay case
....................................................
File
backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/action/MoveDiskParameters.java
Line 6:
please add: private static final long serialVersionUID
....................................................
File
backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/businessentities/DiskImage.java
Line 626:
TEMPLATE should be const.
Also please write "TEMPLATE".equals(parentType)
--
To view, visit http://gerrit.ovirt.org/1608
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I4e3eddcf7b00c65440eac76f141de1e5ba83c675
Gerrit-PatchSet: 2
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Jonathan Choate <[email protected]>
Gerrit-Reviewer: Jonathan Choate <[email protected]>
Gerrit-Reviewer: Michael Kublin <[email protected]>
_______________________________________________
Engine-patches mailing list
[email protected]
http://lists.ovirt.org/mailman/listinfo/engine-patches
