Tal Nisan has posted comments on this change. Change subject: core: Support detach Storage Domain with disks. ......................................................................
Patch Set 1: (3 comments) http://gerrit.ovirt.org/#/c/24286/1/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/storage/DetachStorageDomainFromPoolCommand.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/storage/DetachStorageDomainFromPoolCommand.java: Line 41: @Override Line 42: protected void executeCommand() { Line 43: log.info("Start detach storage domain"); Line 44: if (!canStorageDomainBeDetached()) { Line 45: setSucceeded(Boolean.FALSE.booleanValue()); Why don't we just use false? Line 46: return; Line 47: } Line 48: changeStorageDomainStatusInTransaction(getStorageDomain().getStoragePoolIsoMapData(), Line 49: StorageDomainStatus.Locked); http://gerrit.ovirt.org/#/c/24286/1/backend/manager/modules/dal/src/test/java/org/ovirt/engine/core/dao/VmStaticDAOTest.java File backend/manager/modules/dal/src/test/java/org/ovirt/engine/core/dao/VmStaticDAOTest.java: Line 420: /** Line 421: * Ensures that there are no VMs with disks on different storage domains. Line 422: */ Line 423: @Test Line 424: public void testGetAllVMsWithDisksOnOtherStorageDomain() { What about a test for a positive flow? Line 425: List<String> result = dao.getAllVMsWithDisksOnOtherStorageDomain(FixturesTool.STORAGE_DOAMIN_SCALE_SD5); Line 426: Line 427: assertNotNull(result); Line 428: assertFalse(result.isEmpty()); http://gerrit.ovirt.org/#/c/24286/1/packaging/dbscripts/vms_sp.sql File packaging/dbscripts/vms_sp.sql: Line 999: END; $procedure$ Line 1000: LANGUAGE plpgsql; Line 1001: Line 1002: Line 1003: Create or replace FUNCTION getAllVMsWithDisksOnOtherStorageDomain(v_storage_domain_id UUID) RETURNS SETOF GetNamesOfVmStatic_rs I really dislike this kind of queries that return only a specific part needed for a specific task while their logic can be used for more generic actions, what happens is that later someone will need some other property on the VM such as guid and will have to change this query to fit his needs, fetching the whole vm_static is as easy as fetching the names only, I do think it's better to do it this way and extract the names in the code, there is very little penalty in that, network-wise Line 1004: AS $procedure$ Line 1005: BEGIN Line 1006: RETURN QUERY SELECT DISTINCT vm_static.vm_name Line 1007: FROM vm_static -- To view, visit http://gerrit.ovirt.org/24286 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I971fe6acd4a2667a09487c5e1108cf7c759587f1 Gerrit-PatchSet: 1 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Maor Lipchuk <[email protected]> Gerrit-Reviewer: Allon Mureinik <[email protected]> Gerrit-Reviewer: Daniel Erez <[email protected]> Gerrit-Reviewer: Eli Mesika <[email protected]> Gerrit-Reviewer: Liron Ar <[email protected]> Gerrit-Reviewer: Sergey Gotliv <[email protected]> Gerrit-Reviewer: Tal Nisan <[email protected]> Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes _______________________________________________ Engine-patches mailing list [email protected] http://lists.ovirt.org/mailman/listinfo/engine-patches
