Liron Aravot has posted comments on this change.
Change subject: core: Add scan domain query
......................................................................
Patch Set 1: I would prefer that you didn't submit this
(4 inline comments)
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/ScanDomainQuery.java
Line 47: for (Guid id : imagesList) {
Line 48: diskImageList.put(id, getDbFacade().getDiskDao().get(id));
Line 49: }
Line 50: // retArr is the ArrayList which will be returned by this query
Line 51: ArrayList<Guid> retArr = new ArrayList<Guid>();
won't we fetch the whole disks from the DB when running this line? seems to be
like it will cause for really bad performance..why not fetch only the disks of
the SD from the DB and than compare?
Line 52: List<Disk> allDisks =
DbFacade.getInstance().getDiskDao().getAll();
Line 53: ArrayList<Disk> disksOnSD = new ArrayList<Disk>(); // disks on
the storage domain passed as a parameter
Line 54: // filter disks in allDisks into disksOnSD if they are on this
domain
Line 55: for (Disk d : allDisks) {
Line 51: ArrayList<Guid> retArr = new ArrayList<Guid>();
Line 52: List<Disk> allDisks =
DbFacade.getInstance().getDiskDao().getAll();
Line 53: ArrayList<Disk> disksOnSD = new ArrayList<Disk>(); // disks on
the storage domain passed as a parameter
Line 54: // filter disks in allDisks into disksOnSD if they are on this
domain
Line 55: for (Disk d : allDisks) {
DiskStorageType.IMAGE can't be null, so you can use == instead and avoid
unneeded null check.
Line 56: if (d.getDiskStorageType().equals(DiskStorageType.IMAGE)) {
Line 57: if (((DiskImage)
d).getstorage_ids().contains(getParameters().getDomainId())) {
Line 58: disksOnSD.add(d);
Line 59: }
Line 60: }
Line 61: }
Line 62: // then, compare the list of all images on the domain with the
list oVirt recognizes
Line 63: for (Map.Entry<Guid, Disk> listItem :
diskImageList.entrySet()) {
Line 64: if (!(disksOnSD.contains(listItem.getValue()))) {
I'd offer to use a map.
Line 65: retArr.add(listItem.getKey());
Line 66: }
Line 67: }
Line 68: // log the difference [for testing purposes]
Line 61: }
Line 62: // then, compare the list of all images on the domain with the
list oVirt recognizes
Line 63: for (Map.Entry<Guid, Disk> listItem :
diskImageList.entrySet()) {
Line 64: if (!(disksOnSD.contains(listItem.getValue()))) {
Line 65: retArr.add(listItem.getKey());
if we already fetched and mapped the results to objects, i think that it's
better to return a list of Disk, because the returned Guids would be probably
used for fetching the entities from the DB. no reason in my opinion to return
guid if we already performed load.
Line 66: }
Line 67: }
Line 68: // log the difference [for testing purposes]
Line 69: logger.info("Returning list from ScanDomainQuery");
//$NON-NLS-1$
--
To view, visit http://gerrit.ovirt.org/8070
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I5ada1a6a030c090e872b2eb3f67ee9325f379963
Gerrit-PatchSet: 1
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Ricky Hopper <[email protected]>
Gerrit-Reviewer: Allon Mureinik <[email protected]>
Gerrit-Reviewer: Eduardo <[email protected]>
Gerrit-Reviewer: Federico Simoncelli <[email protected]>
Gerrit-Reviewer: Liron Aravot <[email protected]>
_______________________________________________
Engine-patches mailing list
[email protected]
http://lists.ovirt.org/mailman/listinfo/engine-patches
