Tal Nisan has posted comments on this change.
Change subject: core: Added RegisterDiskCommand and GetUnregisteredDisksQuery
......................................................................
Patch Set 1: (6 inline comments)
Basically looks ok, some adjustments though, check out my comments please.
I haven't looked at the REST part since Michael already gave his comments, I'll
review again when those are fixed
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/GetUnregisteredDiskQuery.java
Line 32: getQueryReturnValue().setSucceeded(true);
Line 33: }
Line 34: }
Line 35:
Line 36: public static DiskImage populateDisk(BackendInternal backend, Guid
storageDomainId, Guid diskId) {
Currently not used outside the class, please change modifier to private
Line 37: Guid storagePoolId = getStoragePoolIdForDomain(backend,
storageDomainId);
Line 38:
Line 39: // Now get the list of volumes for each new image.
Line 40: StoragePoolDomainAndGroupIdBaseVDSCommandParameters
getVolumesParameters = new StoragePoolDomainAndGroupIdBaseVDSCommandParameters(
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/GetUnregisteredDisksQuery.java
Line 41: List<Disk> unregisteredDisks = new ArrayList<Disk>();
Line 42: for (Guid unregisteredDiskId : imagesList) {
Line 43: String sessionId = getParameters().getSessionId();
Line 44: if (sessionId == null) {
Line 45: sessionId =
ThreadLocalParamsContainer.getHttpSessionId();
I don't see what is the purpose of the 3 lines above concerning the sessionId
Line 46: }
Line 47: GetUnregisteredDiskQueryParameters unregQueryParams = new
GetUnregisteredDiskQueryParameters(
Line 48: unregisteredDiskId, getStorageDomainId());
Line 49: VdcQueryReturnValue unregQueryReturn =
getBackend().runInternalQuery(VdcQueryType.GetUnregisteredDisk,
Line 50: unregQueryParams);
Line 51: if (unregQueryReturn.getSucceeded()) {
Line 52: unregisteredDisks.add((Disk)
unregQueryReturn.getReturnValue());
Line 53: } else {
Line 54: System.out.println("Could not get populated disk,
reason: " + unregQueryReturn.getExceptionString());
Please use log instead, sysout should not be used for logging
Line 55: }
Line 56: }
Line 57: getQueryReturnValue().setReturnValue(unregisteredDisks);
Line 58: }
Line 61: return Backend.getInstance().getResourceManager();
Line 62: }
Line 63:
Line 64: protected Guid getStoragePoolId() {
Line 65: return
getDbFacade().getStoragePoolDao().getAllForStorageDomain(getStorageDomainId()).get(0).getId();
Might throw an IndexOutOfBound if no the storage domain is not attached to any
storage pool
Line 66: }
Line 67:
Line 68: protected Guid getStorageDomainId() {
Line 69: return getParameters().getStorageDomainId();
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/RegisterDiskCommand.java
Line 31:
Line 32: @Override
Line 33: protected boolean canDoAction() {
Line 34: // Currently this only supports importing images and does not
work with luns.
Line 35: return getParameters().getDiskImage().getDiskStorageType() ==
DiskStorageType.IMAGE;
It's better to return a message upon canDoAction failure, something like
"Cannot ${action} ${type}, this operation is supported only for image disks"
Line 36: }
Line 37:
Line 38: @Override
Line 39: protected void executeCommand() {
....................................................
File
backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/GetUnregisteredDisksQueryTest.java
Line 34: import org.ovirt.engine.core.dao.DiskImageDAO;
Line 35: import org.ovirt.engine.core.utils.MockConfigRule;
Line 36:
Line 37: @RunWith(MockitoJUnitRunner.class)
Line 38: public class GetUnregisteredDisksQueryTest
The test fails for me on a NPE in GetUnregisteredDisksQuery line 52
Line 39: extends
Line 40: AbstractQueryTest<StorageDomainQueryParametersBase,
GetUnregisteredDisksQuery<StorageDomainQueryParametersBase>> {
Line 41:
Line 42: @Rule
--
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: Ayal Baron <aba...@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
