Allon Mureinik has posted comments on this change.
Change subject: core: Added RegisterDiskCommand and GetUnregisteredDisksQuery
......................................................................
Patch Set 2: I would prefer that you didn't submit this
(17 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) {
why pass a Backend instance? you can just call getBackend()
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 61: } else {
Line 62: RuntimeException exc =
imageInfoReturn.getExceptionObject();
Line 63: if (exc != null) {
Line 64: throw exc;
Line 65: }
It's better to return null and set success=false.
Line 66: }
Line 67: }
Line 68: } else {
Line 69: RuntimeException exc =
volumesListReturn.getExceptionObject();
Line 67: }
Line 68: } else {
Line 69: RuntimeException exc =
volumesListReturn.getExceptionObject();
Line 70: if (exc != null) {
Line 71: throw exc;
here too
Line 72: }
Line 73: }
Line 74: return null;
Line 75: }
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/RegisterDiskCommand.java
Line 11: import
org.ovirt.engine.core.common.businessentities.Disk.DiskStorageType;
Line 12: import org.ovirt.engine.core.compat.Guid;
Line 13: import org.ovirt.engine.core.dal.VdcBllMessages;
Line 14:
Line 15: @InternalCommandAttribute
an internal command is a command called by another command, not directly by an
end user (via UI/REST).
I can't find where this command is called, so IMHO, it should not be an
internal command.
As such, it should override the getPermissionCheckSubjects() method (see
AddDiskCommand for example).
Line 16: @NonTransactiveCommandAttribute(forceCompensation=true)
Line 17: public class RegisterDiskCommand <T extends RegisterDiskParameters>
extends BaseImagesCommand<T> implements QuotaStorageDependent {
Line 18:
Line 19: private static final long serialVersionUID = -1201881996330878181L;
Line 12: import org.ovirt.engine.core.compat.Guid;
Line 13: import org.ovirt.engine.core.dal.VdcBllMessages;
Line 14:
Line 15: @InternalCommandAttribute
Line 16: @NonTransactiveCommandAttribute(forceCompensation=true)
Why @NonTransactional?
Line 17: public class RegisterDiskCommand <T extends RegisterDiskParameters>
extends BaseImagesCommand<T> implements QuotaStorageDependent {
Line 18:
Line 19: private static final long serialVersionUID = -1201881996330878181L;
Line 20:
Line 32: @Override
Line 33: protected boolean canDoAction() {
Line 34: // Currently this only supports importing images and does not
work with luns.
Line 35: if (getParameters().getDiskImage().getDiskStorageType() !=
DiskStorageType.IMAGE)
Line 36: {
I'd push the "{" up a line, but that's just me.
Line 37: addCanDoActionMessage("$diskId " +
getParameters().getDiskImage().getId());
Line 38: addCanDoActionMessage("$storageType " +
getParameters().getDiskImage().getDiskStorageType());
Line 39:
addCanDoActionMessage(VdcBllMessages.ACTION_TYPE_FAILED_UNSUPPORTED_DISK_STORAGE_TYPE);
Line 40: return false;
Line 37: addCanDoActionMessage("$diskId " +
getParameters().getDiskImage().getId());
Line 38: addCanDoActionMessage("$storageType " +
getParameters().getDiskImage().getDiskStorageType());
Line 39:
addCanDoActionMessage(VdcBllMessages.ACTION_TYPE_FAILED_UNSUPPORTED_DISK_STORAGE_TYPE);
Line 40: return false;
Line 41: } else {
I'd unwrap the else clause, but it's a matter of taste.
Line 42: return true;
Line 43: }
Line 44: }
Line 45:
Line 39:
addCanDoActionMessage(VdcBllMessages.ACTION_TYPE_FAILED_UNSUPPORTED_DISK_STORAGE_TYPE);
Line 40: return false;
Line 41: } else {
Line 42: return true;
Line 43: }
consider also adding a check the the destination storage domain exists and is
up.
(Something down the lines of validate(new
StorageDomainValidator(myStorageDomains).isExistsAndActive())
Line 44: }
Line 45:
Line 46: @Override
Line 47: protected void executeCommand() {
Line 45:
Line 46: @Override
Line 47: protected void executeCommand() {
Line 48: final DiskImage newDiskImage = getParameters().getDiskImage();
Line 49: if (newDiskImage != null) {
The null check should be in the canDoAction(), not the executeCommand()
Line 50: addDiskImageToDb(newDiskImage, getCompensationContext());
Line 51:
getReturnValue().setActionReturnValue(newDiskImage.getId());
Line 52: getReturnValue().setSucceeded(true);
Line 53: }
Line 46: @Override
Line 47: protected void executeCommand() {
Line 48: final DiskImage newDiskImage = getParameters().getDiskImage();
Line 49: if (newDiskImage != null) {
Line 50: addDiskImageToDb(newDiskImage, getCompensationContext());
Note: This method performs a couple of DAO calls - currently, you're working in
@NonTrasactiveCommandAttribute, so this may not roll back cleanly.
Line 51:
getReturnValue().setActionReturnValue(newDiskImage.getId());
Line 52: getReturnValue().setSucceeded(true);
Line 53: }
Line 54: }
....................................................
File
backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/GetUnregisteredDisksQueryTest.java
Line 39: extends
Line 40: AbstractQueryTest<GetUnregisteredDisksQueryParameters,
GetUnregisteredDisksQuery<GetUnregisteredDisksQueryParameters>> {
Line 41:
Line 42: @Rule
Line 43: public static final MockConfigRule mcr = new MockConfigRule();
where are you using this?
Line 44:
Line 45: private Guid importDiskId = Guid.NewGuid();
Line 46: private Guid existingDiskId = Guid.NewGuid();
Line 47: private Guid storageDomainId = Guid.NewGuid();
Line 44:
Line 45: private Guid importDiskId = Guid.NewGuid();
Line 46: private Guid existingDiskId = Guid.NewGuid();
Line 47: private Guid storageDomainId = Guid.NewGuid();
Line 48: private Guid storagePoolId = Guid.NewGuid();
All these initializations should be done in the @Before method, not the
(implicit) constructor
Line 49:
Line 50: private List<Guid> importDiskIds = new
ArrayList<Guid>(Arrays.asList(importDiskId, existingDiskId));
Line 51:
Line 52: @Before
Line 46: private Guid existingDiskId = Guid.NewGuid();
Line 47: private Guid storageDomainId = Guid.NewGuid();
Line 48: private Guid storagePoolId = Guid.NewGuid();
Line 49:
Line 50: private List<Guid> importDiskIds = new
ArrayList<Guid>(Arrays.asList(importDiskId, existingDiskId));
why not just Arrays.asList ?
Line 51:
Line 52: @Before
Line 53: public void setUp() throws Exception {
Line 54: super.setUp();
....................................................
File
backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dal/VdcBllMessages.java
Line 716: ACTION_TYPE_FAILED_EXTERNAL_EVENT_NOT_FOUND,
Line 717: ACTION_TYPE_FAILED_EXTERNAL_EVENT_ILLRGAL_OPERATION,
Line 718:
Line 719: // Register disk
Line 720: ACTION_TYPE_FAILED_UNSUPPORTED_DISK_STORAGE_TYPE;
I'd push this up to be next to the other disk-related messages.
Line 721:
Line 722: public int getValue() {
Line 723: return this.ordinal();
Line 724: }
....................................................
File
backend/manager/modules/restapi/jaxrs/src/test/java/org/ovirt/engine/api/restapi/resource/BackendStorageDomainDiskResourceTest.java
Line 69: DiskImage entity = new DiskImage();
Line 70: entity.setId(GUIDS[index]);
Line 71: entity.setvolume_format(VolumeFormat.RAW);
Line 72: entity.setDiskInterface(DiskInterface.VirtIO);
Line 73: entity.setImageStatus(ImageStatus.OK);
This will probably disappear when you rebase.
Line 74: entity.setvolume_type(VolumeType.Sparse);
Line 75: entity.setBoot(false);
Line 76: entity.setShareable(false);
Line 77: entity.setPropagateErrors(PropagateErrors.On);
....................................................
File
backend/manager/modules/restapi/jaxrs/src/test/java/org/ovirt/engine/api/restapi/resource/BackendStorageDomainDisksResourceTest.java
Line 41: DiskImage entity = new DiskImage();
Line 42: entity.setId(GUIDS[index]);
Line 43: entity.setvolume_format(VolumeFormat.RAW);
Line 44: entity.setDiskInterface(DiskInterface.VirtIO);
Line 45: entity.setImageStatus(ImageStatus.OK);
This will probably disappear when you rebase.
Line 46: entity.setvolume_type(VolumeType.Sparse);
Line 47: entity.setBoot(false);
Line 48: entity.setShareable(false);
Line 49: entity.setPropagateErrors(PropagateErrors.On);
....................................................
Commit Message
Line 30: storage domain, I found that it returned a list of storage
Line 31: pools. I am assuming that an NFS based storage domain would
Line 32: only belong to a single storage pool and thus I took the first
Line 33: item in the list. If this assumption is unfounded please note
Line 34: in the review.
As long as it's a data domain (i.e., not an export domain or an ISO domain),
this assumption is correct, regardless of the underlying storage (NFS, iSCSI,
etc.)
Line 35:
Line 36: This change also implements the GET and POST methods for the
Line 37: REST /api/storagedomains/{storagedomain:id}/disks;unregistered
Line 38: resource. These depend on the new RegisterDiskCommand and
--
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
