Liron Ar has posted comments on this change. Change subject: core: added GetAllDiskSnapshotsByStorageDomainIdQuery ......................................................................
Patch Set 4: (3 comments) http://gerrit.ovirt.org/#/c/26323/4/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/GetAllDiskSnapshotsByStorageDomainIdQuery.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/GetAllDiskSnapshotsByStorageDomainIdQuery.java: Line 19: List<DiskImage> diskImages = Line 20: getDbFacade().getDiskImageDao().getAllSnapshotsForStorageDomain(getParameters().getId()); Line 21: Line 22: // Filter out active volumes Line 23: CollectionUtils.filter(diskImages, new Predicate() { rather than using filter, please use select(), our dao layer is currently using arraylists, using filter to remove the unneeded elements will be less effective, therefore select() will better suit here imo. Line 24: @Override Line 25: public boolean evaluate(Object diskImage) { Line 26: return !((DiskImage) diskImage).getActive(); Line 27: } Line 30: // Retrieving snapshots objects for setting description Line 31: List<Snapshot> snapshots = Line 32: getDbFacade().getSnapshotDao().getAllByStorageDomain(getParameters().getId()); Line 33: Line 34: for (final DiskImage diskImage : diskImages) { I suggest to change this part to the following which should be more efficient: Map<Guid, Snapshot> snapshots = Entities.businessEntitiesById(...) for (Diskimage disk : diskImages) { diskImage.setVmSnapshotDescription(snapshot.getDescription()); } Line 35: Snapshot snapshot = (Snapshot) CollectionUtils.find(snapshots, new Predicate() { Line 36: @Override Line 37: public boolean evaluate(Object snapshot) { Line 38: return diskImage.getVmSnapshotId().equals(((Snapshot) snapshot).getId()); http://gerrit.ovirt.org/#/c/26323/4/backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/GetAllDiskSnapshotsByStorageDomainIdQueryTest.java File backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/GetAllDiskSnapshotsByStorageDomainIdQueryTest.java: Line 94: Line 95: // Assert the correct disks are returned Line 96: assertEquals("There should be two images returned", diskImages.size(), 2); Line 97: assertEquals("DiskImage should contain the VmSnapshotDescription", diskImages.get(0).getVmSnapshotDescription(), Line 98: snapshotDescription); I'd suggest to add a test that validates that active images aren't returned Line 99: } -- To view, visit http://gerrit.ovirt.org/26323 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I389579e4120cb33edca594caa762a8bad63362b7 Gerrit-PatchSet: 4 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Daniel Erez <[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: [email protected] Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes _______________________________________________ Engine-patches mailing list [email protected] http://lists.ovirt.org/mailman/listinfo/engine-patches
