Ayal Baron has posted comments on this change.
Change subject: core: Added RegisterDiskCommand and GetUnregisteredDisksQuery
......................................................................
Patch Set 2: (3 inline comments)
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/GetUnregisteredDiskQuery.java
Line 28: getQueryReturnValue().setSucceeded(true);
Line 29: }
Line 30: }
Line 31:
Line 32: public DiskImage populateDisk(BackendInternal backend, Guid
storagePoolId, Guid storageDomainId, Guid diskId) {
still public?
Line 33:
Line 34: // Now get the list of volumes for each new image.
Line 35: StoragePoolDomainAndGroupIdBaseVDSCommandParameters
getVolumesParameters = new StoragePoolDomainAndGroupIdBaseVDSCommandParameters(
Line 36: storagePoolId, storageDomainId, diskId);
Line 35: StoragePoolDomainAndGroupIdBaseVDSCommandParameters
getVolumesParameters = new StoragePoolDomainAndGroupIdBaseVDSCommandParameters(
Line 36: storagePoolId, storageDomainId, diskId);
Line 37: VDSReturnValue volumesListReturn =
backend.getResourceManager().RunVdsCommand(VDSCommandType.GetVolumesList,
Line 38: getVolumesParameters);
Line 39: if (volumesListReturn.getSucceeded()) {
(continuing the discussion from patch set 1 on returning early here) your
'else' is way down below and detached from the logic, plus you already throw
there, so doing the negative first would seem much more legible to me. Plus,
you're verifying the value you just got is ok before proceeding (in general I'd
just have GetVolumesList throw, but that's a different matter) so flow seems
better to me if you throw first then proceed if all is ok.
Line 40: @SuppressWarnings("unchecked")
Line 41: List<Guid> volumesList = (List<Guid>)
volumesListReturn.getReturnValue();
Line 42:
Line 43: // We can't deal with snapshots, so there should only be a
single volume associated with the
Line 41: List<Guid> volumesList = (List<Guid>)
volumesListReturn.getReturnValue();
Line 42:
Line 43: // We can't deal with snapshots, so there should only be a
single volume associated with the
Line 44: // image. If there are multiple volumes, skip the image
and move on to the next.
Line 45: if (volumesList.size() == 1) {
continue on !=1 would still look better here
Line 46: Guid volumeId = volumesList.get(0);
Line 47:
Line 48: // Get the information about the volume from VDSM.
Line 49: GetImageInfoVDSCommandParameters imageInfoParameters =
new GetImageInfoVDSCommandParameters(
--
To view, visit http://gerrit.ovirt.org/11783
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I82de498fd9a8e25ed9e1dc5776f2fdf0c35b46da
Gerrit-PatchSet: 2
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Chris Morrissey <cmorr...@netapp.com>
Gerrit-Reviewer: Allon Mureinik <amure...@redhat.com>
Gerrit-Reviewer: Ayal Baron <aba...@redhat.com>
Gerrit-Reviewer: Chris Morrissey <cmorr...@netapp.com>
Gerrit-Reviewer: Federico Simoncelli <fsimo...@redhat.com>
Gerrit-Reviewer: Itamar Heim <ih...@redhat.com>
Gerrit-Reviewer: Michael Pasternak <mpast...@redhat.com>
Gerrit-Reviewer: Tal Nisan <tni...@redhat.com>
_______________________________________________
Engine-patches mailing list
Engine-patches@ovirt.org
http://lists.ovirt.org/mailman/listinfo/engine-patches
