Allon Mureinik has posted comments on this change. Change subject: core: introudcing domain ovf handling commands ......................................................................
Patch Set 10: Code-Review+1 (9 comments) Looks good to me (see minor comments inline), but I'd like more eyes on this. http://gerrit.ovirt.org/#/c/23529/10//COMMIT_MSG Commit Message: Line 6: Line 7: core: introudcing domain ovf handling commands Line 8: Line 9: *ProccessOvfUpdateForStorageDomainCommand - Line 10: Used for perform ovf update for the valid ovf stores of a storage s/perform/performing/ s/update/updates/ Line 11: domain, creates more ovf stores if needed (there are less than defined Line 12: in the config file) Line 13: Line 14: *CreateOvfVolumeForStorageDomainCommand - http://gerrit.ovirt.org/#/c/23529/10/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/ProccessOvfUpdateForStorageDomainCommand.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/ProccessOvfUpdateForStorageDomainCommand.java: Line 68: ovfDiskCount = storageDomainOvfInfos.size(); Line 69: // Ordering to provide consistent ovf update order - the order should be as follows, so that in case Line 70: // of failure we will have "previous" version of the data on other disk. Line 71: // 1. disks that were never ovf updated (getLastUpdated is null) Line 72: // 1. disk id should be "2." Line 73: Collections.sort(storageDomainOvfInfos, new Comparator<StorageDomainOvfInfo>() { Line 74: @Override Line 75: public int compare(StorageDomainOvfInfo storageDomainOvfInfo, StorageDomainOvfInfo storageDomainOvfInfo2) { Line 76: int compareResult = ObjectUtils.compare(storageDomainOvfInfo.getLastUpdated(), storageDomainOvfInfo2.getLastUpdated()); Line 78: return compareResult; Line 79: } Line 80: return storageDomainOvfInfo.getOvfDiskId().compareTo(storageDomainOvfInfo2.getOvfDiskId()); Line 81: } Line 82: }); I'd extract this to an inner class with a sensible name. IMHO, easier to read that way. Line 83: Line 84: for (StorageDomainOvfInfo storageDomainOvfInfo : storageDomainOvfInfos) { Line 85: if (storageDomainOvfInfo.getStatus() != StorageDomainOvfInfoStatus.DISABLED) { Line 86: DiskImage ovfDisk = (DiskImage) getDbFacade().getDiskDao().get(storageDomainOvfInfo.getOvfDiskId()); Line 88: } Line 89: } Line 90: } Line 91: Line 92: private void loadStorageDomain() { why not a lazy getter? why assume you go through the CDA? Line 93: storageDomain = getDbFacade().getStorageDomainDao().getForStoragePool(getParameters().getStorageDomainId(), getParameters().getStoragePoolId()); Line 94: } Line 95: Line 96: Line 92: private void loadStorageDomain() { Line 93: storageDomain = getDbFacade().getStorageDomainDao().getForStoragePool(getParameters().getStorageDomainId(), getParameters().getStoragePoolId()); Line 94: } Line 95: Line 96: TWS Line 97: private byte[] buildOvfInfoByteArrayOfVmAndTemplates(List<Guid> vmAndTemplatesIds) { Line 98: ByteArrayOutputStream bufferedOutputStream = new ByteArrayOutputStream(); Line 99: Line 100: try (InMemoryTar inMemoryTar = new InMemoryTar(bufferedOutputStream)) { Line 114: } catch (Exception e) { Line 115: throw new RuntimeException(String.format("Exception while building in memory tar of the OVFs of domain %s", Line 116: getParameters().getStorageDomainId()), e); Line 117: } Line 118: TWS Line 119: return bufferedOutputStream.toByteArray(); Line 120: } Line 121: Line 122: protected void updateOvfStoreContent() { Line 145: iterationLimit--; Line 146: } Line 147: Line 148: boolean shouldUpdateLastOvfStore = false; Line 149: TWS Line 150: if (iterationLimit == 0) { Line 151: // if we have only one ovf store, we should update it. Line 152: shouldUpdateLastOvfStore = true; Line 153: } else { Line 206: @Override Line 207: protected void executeCommand() { Line 208: Integer ovfStoresCountForDomain = Config.<Integer> getValue(ConfigValues.StorageDomainOvfStoreCount); Line 209: Line 210: for (int i=ovfDiskCount; i<ovfStoresCountForDomain; i++){ formatter Line 211: Backend.getInstance().runInternalAction(VdcActionType.CreateOvfVolumeForStorageDomain, Line 212: getParameters()); Line 213: } Line 214: http://gerrit.ovirt.org/#/c/23529/10/backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/constants/StorageConstants.java File backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/constants/StorageConstants.java: Line 1: package org.ovirt.engine.core.common.constants; Line 2: Line 3: public class StorageConstants { Line 4: public static final int SIZE_IS_NOT_AVAILABLE = -1; Line 5: public static final int OVF_MAX_ITEMS_PER_SQL_STATEMENT = 100; This should be a config value (can be done in a different patch) -- To view, visit http://gerrit.ovirt.org/23529 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ifea111e6732ec0b6953d54ef03c29eb0dff6f837 Gerrit-PatchSet: 10 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Liron Ar <[email protected]> Gerrit-Reviewer: Allon Mureinik <[email protected]> Gerrit-Reviewer: Daniel Erez <[email protected]> Gerrit-Reviewer: Liron Ar <[email protected]> Gerrit-Reviewer: Maor Lipchuk <[email protected]> Gerrit-Reviewer: Tal Nisan <[email protected]> Gerrit-Reviewer: [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
