Ori Liel has posted comments on this change. Change subject: rest: retrieve disk snapshots list by storage domain ......................................................................
Patch Set 5: (6 comments) http://gerrit.ovirt.org/#/c/26730/5/backend/manager/modules/restapi/jaxrs/src/main/java/org/ovirt/engine/api/restapi/resource/BackendStorageDomainDiskSnapshotResource.java File backend/manager/modules/restapi/jaxrs/src/main/java/org/ovirt/engine/api/restapi/resource/BackendStorageDomainDiskSnapshotResource.java: Line 17: this.storageDomainId = parent.getStorageDomainId().toString(); Line 18: } Line 19: Line 20: @Override Line 21: protected DiskSnapshot deprecatedPopulate(DiskSnapshot model, org.ovirt.engine.core.common.businessentities.Disk entity) { same comment as in BackendStorageDomainDiskSnapshotsResource.java Line 22: DiskSnapshot populatedDisk = doPopulate(model, entity); Line 23: Line 24: // this code generates back-link to the corresponding SD Line 25: populatedDisk.setStorageDomain(new StorageDomain()); http://gerrit.ovirt.org/#/c/26730/5/backend/manager/modules/restapi/jaxrs/src/main/java/org/ovirt/engine/api/restapi/resource/BackendStorageDomainDiskSnapshotsResource.java File backend/manager/modules/restapi/jaxrs/src/main/java/org/ovirt/engine/api/restapi/resource/BackendStorageDomainDiskSnapshotsResource.java: Line 32: @Override Line 33: protected DiskSnapshot addLinks(DiskSnapshot model, Line 34: Class<? extends BaseResource> suggestedParent, Line 35: String... excludeSubCollectionMembers) { Line 36: return super.addLinks(model, StorageDomain.class, excludeSubCollectionMembers); why necessary to override addLinks()? Line 37: } Line 38: Line 39: @Override Line 40: protected DiskSnapshot addLinks(DiskSnapshot model, String... subCollectionMembersToExclude) { Line 37: } Line 38: Line 39: @Override Line 40: protected DiskSnapshot addLinks(DiskSnapshot model, String... subCollectionMembersToExclude) { Line 41: return addLinks(model, StorageDomain.class, subCollectionMembersToExclude); why necessary to override addLinks()? Line 42: } Line 43: Line 44: @Override Line 45: public DiskSnapshots list() { Line 49: Line 50: protected DiskSnapshots mapCollection(List<org.ovirt.engine.core.common.businessentities.Disk> entities) { Line 51: DiskSnapshots collection = new DiskSnapshots(); Line 52: for (org.ovirt.engine.core.common.businessentities.Disk disk : entities) { Line 53: DiskSnapshot diskSnapshot = DiskSnapshotMapper.map(disk, null); Consider using the mapping locator: getMapper(org.ovirt.engine.core.common.businessentities.Disk.class, DiskSnapshot.class).map(disk, null); Client code asks for a mapper that maps from X.class to Y.class, and the infrasctructure provides it, saving the client the need to know in which mapper class (DiskSnapshotMapper in this case) the mapping is implemented. Not a must. Line 54: collection.getDiskSnapshots().add(addLinks(populate(diskSnapshot, disk))); Line 55: } Line 56: return collection; Line 57: } Line 57: } Line 58: Line 59: @Override Line 60: protected Response performRemove(String id) { Line 61: DiskImage diskImage = (DiskImage) DiskSnapshotMapper.map(getDeviceSubResource(id).get(), null); 1) is getDeviceSubResource(id).get() really necessary? you already have the id of the disk. Maybe we can avoid an additional query? 2) Like above, consider using the mapping locator Line 62: return performAction(VdcActionType.RemoveDiskSnapshots, new RemoveDiskSnapshotsParameters(diskImage)); Line 63: } Line 64: Line 65: @Override Line 71: protected DiskSnapshot deprecatedPopulate(DiskSnapshot model, org.ovirt.engine.core.common.businessentities.Disk entity) { Line 72: DiskSnapshot populatedDisk = doPopulate(model, entity); Line 73: Line 74: // this code generates back-link to the corresponding SD Line 75: populatedDisk.setStorageDomain(new StorageDomain()); Move this code into list(). Populate is for extra information, that the user can choose to retrieve or not retrieve using the http-header: "All-Content". Since we always want the disk snapshot to have the back-link to the storage-domain, setting the SD doesn't belong in populate. * P.S - deprecatedPopulate() is deprecated' It exists to support backwards compatibility, and nothing new should be added to it, ever :) Line 76: populatedDisk.getStorageDomain().setId(this.storageDomainId.toString()); Line 77: Line 78: return model; Line 79: } -- To view, visit http://gerrit.ovirt.org/26730 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I5359c5df490c7e7e6f9b4d25e794bd07c6877339 Gerrit-PatchSet: 5 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Daniel Erez <de...@redhat.com> Gerrit-Reviewer: Daniel Erez <de...@redhat.com> Gerrit-Reviewer: Juan Hernandez <juan.hernan...@redhat.com> Gerrit-Reviewer: Ori Liel <ol...@redhat.com> Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes _______________________________________________ Engine-patches mailing list Engine-patches@ovirt.org http://lists.ovirt.org/mailman/listinfo/engine-patches
