Yair Zaslavsky has posted comments on this change.
Change subject: userportal : Fix DiskForVmGuid high CPU
......................................................................
Patch Set 1: I would prefer that you didn't submit this
(6 inline comments)
I like the idea, but the patch is a bit raw.
Really simple comments from my side.
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/GetAllDisksPartialDataByVmIdQuery.java
Line 21: protected void executeQueryCommand() {
Line 22: List<Disk> allDisks =
Line 23: getDbFacade().getDiskDao().getAllForVmPartialData
Line 24: (getParameters().getId(), false, getUserID(),
getParameters().isFiltered());
Line 25: // Set<Guid> pluggedDiskIds = getPluggedDiskIds();
Feel free to remove the comments.
Line 26: // List<Disk> disks = new ArrayList<Disk>(allDisks);
Line 27: // for (Disk disk : allDisks) {
Line 28: // disk.setPlugged(pluggedDiskIds.contains(disk.getId()));
Line 29: // if (disk.getDiskStorageType() == DiskStorageType.IMAGE) {
Line 34: // getQueryReturnValue().setReturnValue(disks);
Line 35: getQueryReturnValue().setReturnValue(allDisks);
Line 36: }
Line 37:
Line 38: protected List<DiskImage> getAllImageSnapshots(DiskImage
diskImage) {
feel free to remove this method.
Line 39: return
ImagesHandler.getAllImageSnapshots(diskImage.getImageId(),
diskImage.getImageTemplateId());
Line 40: }
Line 41:
Line 42: private Set<Guid> getPluggedDiskIds() {
Line 38: protected List<DiskImage> getAllImageSnapshots(DiskImage
diskImage) {
Line 39: return
ImagesHandler.getAllImageSnapshots(diskImage.getImageId(),
diskImage.getImageTemplateId());
Line 40: }
Line 41:
Line 42: private Set<Guid> getPluggedDiskIds() {
feel free to remove this method.
Line 43: List<VmDevice> disksVmDevices =
Line 44: getDbFacade().getVmDeviceDao()
Line 45:
.getVmDeviceByVmIdTypeAndDevice(getParameters().getId(),
Line 46: VmDeviceGeneralType.DISK,
....................................................
File
backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dao/DiskDao.java
Line 115: * @param isFiltered
Line 116: * Whether the results should be filtered according to
the user's permissions
Line 117: * @return the list of disks
Line 118: */
Line 119: List<Disk> getAllForVmPartialData(Guid id, boolean
onlyPluggedDisks, Guid userID, boolean isFiltered);
Please add unit test.
....................................................
File
frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/userportal/VmBasicDiskListModel.java
Line 46: public void onSuccess(Object model, Object ReturnValue)
Line 47: {
Line 48: List<DiskImageBasicView> disks =
Line 49: (List<DiskImageBasicView>)
((VdcQueryReturnValue) ReturnValue).getReturnValue();
Line 50: ArrayList<DiskImageBasicView> diskList = new
ArrayList<DiskImageBasicView>();
Please pass disks to disksList CTOR.
Can you elaborate why you need disks and disksList, if they are lists
(ArrayList extends a list) of the same type?
Line 51: diskList.addAll(disks);
Line 52: Collections.sort(diskList, new
Linq.DiskByAliasComparer());
Line 53:
Line 54: SearchableListModel searchableListModel =
(SearchableListModel) model;
....................................................
File packaging/dbscripts/all_disks_sp.sql
Line 57: Create or replace FUNCTION GetDisks_VmGuid_BasicView(v_vm_guid UUID,
v_only_plugged BOOLEAN, v_user_id UUID, v_is_filtered BOOLEAN)
Line 58: RETURNS SETOF GetDisks_VmGuid_BasicView_rs
Line 59: AS $procedure$
Line 60: BEGIN
Line 61: RETURN QUERY
Lots of trailing whitespaces.
Also, we are trying to enforce some conentions, keywords like select, from,
etc.. should be UPPER case, the from should be under select, the subquery
select should be in new line.
Allon, feel free to elaborate.
Basically, I like this approach.
Line 62: select disk_id,disk_alias, size
Line 63: from images
Line 64: LEFT OUTER JOIN base_disks ON images.image_group_id =
base_disks.disk_id
Line 65: LEFT JOIN vm_device ON vm_device.device_id =
image_group_id AND (NOT v_only_plugged OR is_plugged)
--
To view, visit http://gerrit.ovirt.org/16657
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Iceb6e518936f65403561703e6b70920fbcc1f07c
Gerrit-PatchSet: 1
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Liran Zelkha <[email protected]>
Gerrit-Reviewer: Allon Mureinik <[email protected]>
Gerrit-Reviewer: Daniel Erez <[email protected]>
Gerrit-Reviewer: Eli Mesika <[email protected]>
Gerrit-Reviewer: Yair Zaslavsky <[email protected]>
Gerrit-Reviewer: oVirt Jenkins CI Server
_______________________________________________
Engine-patches mailing list
[email protected]
http://lists.ovirt.org/mailman/listinfo/engine-patches
