Alissa Bonas has posted comments on this change.
Change subject: core: UpdateVmCommand: Shared disk boot status
......................................................................
Patch Set 1: (4 inline comments)
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/UpdateVmDiskCommand.java
Line 40: private static final long serialVersionUID = 5915267156998835363L;
Line 41: private List<PermissionSubject> listPermissionSubjects;
Line 42: protected final Disk oldDisk;
Line 43: protected final Disk newDisk;
Line 44: private Map<Guid, List<Disk>> otherVmDisks = new HashMap<Guid,
List<Disk>>();
it will be nice to add a brief comment (or rename the member itself to a more
clear name) what this map's key represents. it's probably clear from reading
the logic below, but nice to understand from definition as well.
Line 45: private Map<String, Pair<String, String>> sharedLockMap;
Line 46: private Map<String, Pair<String, String>> exclusiveLockMap;
Line 47:
Line 48: public UpdateVmDiskCommand(T parameters) {
Line 147:
Line 148: protected List<Disk> getOtherVmDisks(Guid vmId) {
Line 149: List<Disk> disks = otherVmDisks.get(vmId);
Line 150: if (disks == null) {
Line 151: disks = getDiskDao().getAllForVm(vmId);
If I understand the change correctly, now this code goes to db in loop as
number of VMs, while previously it did it only once for current VM.
Won't it affect performance while user's waiting for response in UI?
What is the expected number of VMs (the size of the for loop iterations) ?
Is there a possibility to extract all of the disks from db at once and do the
sorting in memory?
Line 152: disks.removeAll(LinqUtils.filter(disks, new
Predicate<Disk>() {
Line 153: @Override
Line 154: public boolean eval(Disk o) {
Line 155: return o.getId().equals(oldDisk.getId());
Line 150: if (disks == null) {
Line 151: disks = getDiskDao().getAllForVm(vmId);
Line 152: disks.removeAll(LinqUtils.filter(disks, new
Predicate<Disk>() {
Line 153: @Override
Line 154: public boolean eval(Disk o) {
Can you rename (I know it was here before) the strange disk "o" to a better
name?
Line 155: return o.getId().equals(oldDisk.getId());
Line 156: }
Line 157: }));
Line 158: otherVmDisks.put(vmId, disks);
....................................................
Commit Message
Line 6:
Line 7: core: UpdateVmCommand: Shared disk boot status
Line 8:
Line 9: Check the boot status of a disk against all the VMs it's attached to,
Line 10: not only the one it's context is edited in.
it's -> its
Line 11:
Line 12: Change-Id: I6a9d2e8800b9bf693fc55d8a627e0725277985ac
Line 13: Bug-Url: https://bugzilla.redhat.com/854964
--
To view, visit http://gerrit.ovirt.org/12889
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I6a9d2e8800b9bf693fc55d8a627e0725277985ac
Gerrit-PatchSet: 1
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Allon Mureinik <[email protected]>
Gerrit-Reviewer: Alissa Bonas <[email protected]>
Gerrit-Reviewer: Allon Mureinik <[email protected]>
Gerrit-Reviewer: Daniel Erez <[email protected]>
Gerrit-Reviewer: Liron Ar <[email protected]>
Gerrit-Reviewer: Maor Lipchuk <[email protected]>
Gerrit-Reviewer: Tal Nisan <[email protected]>
Gerrit-Reviewer: Vered Volansky <[email protected]>
_______________________________________________
Engine-patches mailing list
[email protected]
http://lists.ovirt.org/mailman/listinfo/engine-patches
