Tal Nisan has posted comments on this change. Change subject: engine: Add support for Refresh LUNs size ......................................................................
Patch Set 8: (6 comments) https://gerrit.ovirt.org/#/c/39318/8/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/storage/RefreshLunsSizeCommand.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/storage/RefreshLunsSizeCommand.java: This new command introduced needs a test, at least for the CDA part Line 1: package org.ovirt.engine.core.bll.storage; Line 2: Line 3: import java.util.ArrayList; Line 4: import java.util.HashMap; Line 62: addCanDoActionMessage(VdcBllMessages.ACTION_TYPE_FAILED_STORAGE_DOMAIN_TYPE_ILLEGAL); Line 63: return false; Line 64: } Line 65: Line 66: if (!checkStorageDomainStatusNotEqual(StorageDomainStatus.Locked)) { This part seems redundant, you already verified before that the status is active, why check again if it's not locked? Line 67: return false; Line 68: } Line 69: Line 70: if (!checkLunsInStorageDomain(getParameters().getLunIds(), getStorageDomain())) { Line 93: } Line 94: Line 95: @Override Line 96: protected void executeCommand() { Line 97: executeInNewTransaction(new TransactionMethod<Void>() { Since you extracted the status change to active to a methods I'd extract that also to lockStorageDomain or a similar method in StorageDomainCommandBase (since ExtendSANStorageDomainCommand can use it also as the code there is exactly the same) Line 98: public Void runInTransaction() { Line 99: setStorageDomainStatus(StorageDomainStatus.Locked, getCompensationContext()); Line 100: getCompensationContext().stateChanged(); Line 101: return null; Line 112: ArrayList<String> failedVds = new ArrayList<>(); Line 113: for (Map.Entry<String, List<VDS>> entry : lunToFailedVDS.entrySet()) { Line 114: String lunId = entry.getKey(); Line 115: List<VDS> vdsList = entry.getValue(); Line 116: log.error("Failed to refresh device " + lunId + " Not all VDS are seeing the same size " + Might be worth to add an audit log for that Line 117: "VDS :" + vdsList); Line 118: String vdsListString = StringUtils.join(getVdsNameList(vdsList), ", "); //$NON-NLS-1$ Line 119: failedVds.add("LUN :" + lunId + "VDS: " + vdsListString); Line 120: } Line 198: } Line 199: return lunToVds; Line 200: } Line 201: Line 202: private Integer getSize(Map<String, LUNs> allLuns, String lunId) { Although it's private I'd refrain from using a method name like getSize as it's very obfuscated, perhaps getLunSize? Line 203: Line 204: Integer size; Line 205: LUNs lun = allLuns.get(lunId); Line 206: HashMap<String, Integer> pathsCapacity = lun.getPathsCapacity(); Line 205: LUNs lun = allLuns.get(lunId); Line 206: HashMap<String, Integer> pathsCapacity = lun.getPathsCapacity(); Line 207: if(!pathsCapacity.isEmpty()){ Line 208: size = pathsCapacity.values().iterator().next(); Line 209: }else { A space is missing between the left bracket and the else Line 210: size = lun.getDeviceSize(); Line 211: } Line 212: return size; Line 213: } -- To view, visit https://gerrit.ovirt.org/39318 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I6c5c6d59087736466ee5e7eb0c77ee9282199c62 Gerrit-PatchSet: 8 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Freddy Rolland <froll...@redhat.com> Gerrit-Reviewer: Ala Hino <ah...@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: Fred Rolland <froll...@redhat.com> Gerrit-Reviewer: Freddy Rolland <froll...@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: Nir Soffer <nsof...@redhat.com> Gerrit-Reviewer: Ori Liel <ol...@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
