Federico Simoncelli has posted comments on this change.
Change subject: core: Added RegisterDiskCommand and GetUnregisteredDisksQuery
......................................................................
Patch Set 1: (5 inline comments)
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/GetUnregisteredDiskQuery.java
Line 40: StoragePoolDomainAndGroupIdBaseVDSCommandParameters
getVolumesParameters = new StoragePoolDomainAndGroupIdBaseVDSCommandParameters(
Line 41: storagePoolId, storageDomainId, diskId);
Line 42: VDSReturnValue volumesListReturn =
backend.getResourceManager().RunVdsCommand(VDSCommandType.GetVolumesList,
Line 43: getVolumesParameters);
Line 44: if (volumesListReturn.getSucceeded()) {
Just a style note, I'd rather throw early (!volumesListReturn.getSucceeded())
and spare an indentation for the rest of the code.
Line 45: @SuppressWarnings("unchecked")
Line 46: List<Guid> volumesList = (List<Guid>)
volumesListReturn.getReturnValue();
Line 47:
Line 48: // We can't deal with snapshots, so there should only be a
single volume associated with the
Line 46: List<Guid> volumesList = (List<Guid>)
volumesListReturn.getReturnValue();
Line 47:
Line 48: // We can't deal with snapshots, so there should only be a
single volume associated with the
Line 49: // image. If there are multiple volumes, skip the image
and move on to the next.
Line 50: if (volumesList.size() == 1) {
Same thing here probably, if (volumesList.size() != 1) continue. It would be
even more coherent with the comment.
Line 51: Guid volumeId = volumesList.get(0);
Line 52:
Line 53: // Get the information about the volume from VDSM.
Line 54: GetImageInfoVDSCommandParameters imageInfoParameters =
new GetImageInfoVDSCommandParameters(
Line 55: storagePoolId, storageDomainId, diskId,
volumeId);
Line 56: VDSReturnValue imageInfoReturn =
backend.getResourceManager().RunVdsCommand(
Line 57: VDSCommandType.GetImageInfo,
imageInfoParameters);
Line 58:
Line 59: if (imageInfoReturn.getSucceeded()) {
Another indentation that can be spared.
Line 60: DiskImage newDiskImage = (DiskImage)
imageInfoReturn.getReturnValue();
Line 61: // The disk image won't have an interface set on
it. Set it to IDE by default. When the
Line 62: // disk is attached to a VM, its interface can be
changed to the appropriate value for that VM.
Line 63: newDiskImage.setDiskInterface(DiskInterface.IDE);
Line 82: protected static Guid getStoragePoolIdForDomain(BackendInternal
backend, Guid storageDomainId) {
Line 83: // Retrieve the storage pools for the storage domain. This is
needed when getting the list of volumes in the
Line 84: // unregistered images we find in the domain.
Line 85: VdcQueryReturnValue returnValue =
backend.runInternalQuery(VdcQueryType.GetStoragePoolsByStorageDomainId,
Line 86: new StorageDomainQueryParametersBase(storageDomainId));
I'm not expert in this area but I feel like this is not the best practice to
find what you're looking for. Maybe you can go directly to:
getStoragePoolDao().getAllForStorageDomain(storageDomainId);
Even though it might break encapsulation and generate code duplication (but
still it's a one-liner if it works).
Line 87: @SuppressWarnings("unchecked")
Line 88: List<storage_pool> storagePools = (List<storage_pool>)
returnValue.getReturnValue();
Line 89:
Line 90: if (storagePools != null && !storagePools.isEmpty()) {
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/GetUnregisteredDisksQuery.java
Line 44: if (sessionId == null) {
Line 45: sessionId =
ThreadLocalParamsContainer.getHttpSessionId();
Line 46: }
Line 47: GetUnregisteredDiskQueryParameters unregQueryParams = new
GetUnregisteredDiskQueryParameters(
Line 48: unregisteredDiskId, getStorageDomainId());
Since you're already computing the storage pool here, why not add it as a
required parameter in GetUnregisteredDiskQueryParameters and get rid of the
duplicated logic (getStoragePoolId in GetUnregisteredDiskQuery).
Line 49: VdcQueryReturnValue unregQueryReturn =
getBackend().runInternalQuery(VdcQueryType.GetUnregisteredDisk,
Line 50: unregQueryParams);
Line 51: if (unregQueryReturn.getSucceeded()) {
Line 52: unregisteredDisks.add((Disk)
unregQueryReturn.getReturnValue());
--
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: 1
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
