Allon Mureinik has posted comments on this change. Change subject: core: Preventing moving a shareable disk to a Gluster domain ......................................................................
Patch Set 2: Code-Review-1 (6 comments) http://gerrit.ovirt.org/#/c/35398/2/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/MoveDisksCommand.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/MoveDisksCommand.java: Line 71: @Override Line 72: protected boolean canDoAction() { Line 73: if (getParameters().getParametersList().isEmpty()) { Line 74: return failCanDoAction(VdcBllMessages.ACTION_TYPE_FAILED_NO_DISKS_SPECIFIED); Line 75: } else { The else is redundant - unwrap it (project's style) Line 76: return verifyShareableDisksMoving(); Line 77: } Line 78: } Line 79: Line 77: } Line 78: } Line 79: Line 80: private boolean verifyShareableDisksMoving() { Line 81: HashMap<Guid, StorageDomain> targetDomains= new HashMap<>(); Should be defined as a Map - the Hash implementation is inconsequential. Line 82: ArrayList<String> problematicDisks = new ArrayList<>(); Line 83: for ( MoveDiskParameters param : getParameters().getParametersList() ) { Line 84: // fail if there are shareable disks that are being moved to a gluster domain. Line 85: DiskImage currDisk = getDiskImageDao().get(param.getImageId()); Line 78: } Line 79: Line 80: private boolean verifyShareableDisksMoving() { Line 81: HashMap<Guid, StorageDomain> targetDomains= new HashMap<>(); Line 82: ArrayList<String> problematicDisks = new ArrayList<>(); Should be defined as a List. Also, since you're just adding elements to the end and iterating - use a LinkedList. Line 83: for ( MoveDiskParameters param : getParameters().getParametersList() ) { Line 84: // fail if there are shareable disks that are being moved to a gluster domain. Line 85: DiskImage currDisk = getDiskImageDao().get(param.getImageId()); Line 86: if (currDisk.isShareable()) { Line 95: } Line 96: } Line 97: if (!problematicDisks.isEmpty()) { Line 98: addCanDoActionMessage(VdcBllMessages.ACTION_TYPE_FAILED_CANT_MOVE_SHAREABLE_DISK_TO_GLUSTERFS); Line 99: addCanDoActionMessageVariable("problematicDisks", StringHelper.join(", ", problematicDisks.toArray())); //$NON-NLS-1$ Don't we have a utility for multi-part CDA messages? Line 100: return false; Line 101: } else { Line 102: return true; Line 103: } http://gerrit.ovirt.org/#/c/35398/2/backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/MoveDisksCommandTest.java File backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/MoveDisksCommandTest.java: Line 162: initDiskImage(diskImageId, true); Line 163: initStorageDomain(dstStorageId, StorageType.GLUSTERFS); Line 164: Line 165: command.updateParameters(); Line 166: assertFalse(command.canDoAction()); Looking forwards, this is unelegant - it's much better to use CanDoActionTestIUtils. However, it seems like the entire file looks like this - let's keep convention Line 167: } Line 168: Line 169: Line 170: @Test http://gerrit.ovirt.org/#/c/35398/2/backend/manager/modules/dal/src/main/resources/bundles/AppErrors.properties File backend/manager/modules/dal/src/main/resources/bundles/AppErrors.properties: Line 1017: ACTION_TYPE_FAILED_DISKS_NOT_EXIST=Cannot ${action} ${type}. The following disk(s) ID(s) does not exist: ${diskIds}. Line 1018: ACTION_TYPE_FAILED_DISK_SNAPSHOTS_NOT_EXIST=Cannot ${action} ${type}. The following disk snapshot(s) ID(s) does not exist: ${diskSnapshotIds}. Line 1019: ACTION_TYPE_FAILED_DISK_SNAPSHOTS_ACTIVE=Cannot ${action} ${type}. The following disk snapshot(s) is active: ${diskSnapshotIds}. Line 1020: ACTION_TYPE_FAILED_NO_DISKS_SPECIFIED=Cannot ${action} ${type}. No disks have been specified. Line 1021: ACTION_TYPE_FAILED_CANT_MOVE_SHAREABLE_DISK_TO_GLUSTERFS=Cannot ${action} ${type}. Gluster domain does not support the following shareable disks: ${problematicDisks}. How about "Gluster domains do not support shareable disks. The following disks cannot be moved: ${problematicDisks}" ? Line 1022: ACTION_TYPE_FAILED_DISK_IS_NOT_VM_DISK=Cannot ${action} ${type}. The following disk(s) are not attached to any VM: ${diskAliases}. Line 1023: ACTION_TYPE_FAILED_DISK_IS_NOT_TEMPLATE_DISK=Cannot ${action} ${type}. The selected disk is not a template disk. Only template disks can be copied. Line 1024: ACTION_TYPE_FAILED_SOURCE_AND_TARGET_SAME=Cannot ${action} ${type}. The source and target storage domains are the same. Line 1025: ACTION_TYPE_FAILED_CANNOT_MOVE_TEMPLATE_DISK=Cannot ${action} ${type}. Template disks cannot be moved. -- To view, visit http://gerrit.ovirt.org/35398 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I37e500e86d2b6094bb257221a4ca899590aed610 Gerrit-PatchSet: 2 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Amit Aviram <aavi...@redhat.com> Gerrit-Reviewer: Allon Mureinik <amure...@redhat.com> Gerrit-Reviewer: Amit Aviram <aavi...@redhat.com> Gerrit-Reviewer: Daniel Erez <de...@redhat.com> Gerrit-Reviewer: Idan Shaby <ish...@redhat.com> Gerrit-Reviewer: Liron Aravot <lara...@redhat.com> Gerrit-Reviewer: Maor Lipchuk <mlipc...@redhat.com> Gerrit-Reviewer: Tal Nisan <tni...@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
