Greg Padgett has posted comments on this change. Change subject: engine: Add support for Refresh LUNs size ......................................................................
Patch Set 8: (14 comments) Nice! Mostly nits and a couple potential issues for error cases. 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: Line 84: for (LUNs lun : lunsFromDb) { Line 85: if (lunIds.contains(lun.getLUN_id())) { Line 86: if (sdGuid.equals(lun.getStorageDomainId())) { Line 87: // LUN is part of the storage domain Line 88: lunsFoundInSD.add(lun); Might as well return false if you find any LUNs in the wrong SD Line 89: } Line 90: } Line 91: } Line 92: return lunsFoundInSD.size() == lunIds.size(); Line 115: List<VDS> vdsList = entry.getValue(); Line 116: log.error("Failed to refresh device " + lunId + " Not all VDS are seeing the same size " + Line 117: "VDS :" + vdsList); Line 118: String vdsListString = StringUtils.join(getVdsNameList(vdsList), ", "); //$NON-NLS-1$ Line 119: failedVds.add("LUN :" + lunId + "VDS: " + vdsListString); Spacing, something like: "LUN: " + lunId + " VDS: " + vdsListString Line 120: } Line 121: Line 122: changeStorageDomainToActive(); Line 123: throw new VdcBLLException(VdcBllErrors.REFRESH_LUN_ERROR, Line 120: } Line 121: Line 122: changeStorageDomainToActive(); Line 123: throw new VdcBLLException(VdcBllErrors.REFRESH_LUN_ERROR, Line 124: "Failed to refresh LUNs. Not all VDS are seeing the same size " + failedVds); Add a colon? "Failed [...] size: " Line 125: } Line 126: Line 127: // Call PVs resize on SPM Line 128: resizePVs(lunsToRefresh); Line 200: } Line 201: Line 202: private Integer getSize(Map<String, LUNs> allLuns, String lunId) { Line 203: Line 204: Integer size; Any reason to prefer Integer to int for this method? 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 203: Line 204: Integer size; Line 205: LUNs lun = allLuns.get(lunId); Line 206: HashMap<String, Integer> pathsCapacity = lun.getPathsCapacity(); Line 207: if(!pathsCapacity.isEmpty()){ Similar to Tal's comment on 209, missing space after if Line 208: size = pathsCapacity.values().iterator().next(); Line 209: }else { Line 210: size = lun.getDeviceSize(); Line 211: } Line 222: GetDeviceListVDSCommandParameters parameters = Line 223: new GetDeviceListVDSCommandParameters(spmVds.getId(), getStorageDomain().getStorageType()); Line 224: Line 225: Map<String, LUNs> lunsMap = new HashMap<>(); Line 226: List<LUNs> luns = (List<LUNs>) runVdsCommand(VDSCommandType.GetDeviceList, parameters).getReturnValue(); I usually see code doing a vdsReturnValue.getSucceeded() check before the return value is used. Is this not needed here? (Same for other instances of runVdsCommand in this file) Line 227: for (LUNs lun : luns) { Line 228: lunsMap.put(lun.getLUN_id(), lun); Line 229: } Line 230: return lunsMap; Line 244: Line 245: private void resizePVs(List<String> lunsToRefresh) { Line 246: for (String lun : lunsToRefresh) { Line 247: Long pvSize = resizePV(lun); Line 248: log.debug("PV size after resize " + lun + " :" + pvSize); Spacing/wording nits, maybe "PV size after resize of LUN " + lun + ": " + pvSize + "bytes" (Assuming bytes b/c of the Long, but is that right? My personal preference is to add size units where appropriate, which would help people like me who aren't as familiar with the code to understand it more easily without having to hunt around. E.g. a variable name pgSizeInGiB or method getSizeInBytes. After all, http://en.wikipedia.org/wiki/Mars_Climate_Orbiter#Cause_of_failure :)) Line 249: } Line 250: } Line 251: Line 252: private Long resizePV(String lunId) { Line 266: if (returnValueUpdatedStorageDomain.getSucceeded()) { Line 267: StorageDomain updatedStorageDomain = Line 268: (StorageDomain) returnValueUpdatedStorageDomain.getReturnValue(); Line 269: updateStorageDomain(updatedStorageDomain); Line 270: } Do you need an else clause, or does the code expect an exception from runVdsCommand, in which case do you need to check getSucceeded()? Line 271: } Line 272: Line 273: protected VDSReturnValue getStatsForDomain() { Line 274: VDS spmVds = LinqUtils.first(LinqUtils.filter(getAllRunningVdssInPool(), new Predicate<VDS>() { https://gerrit.ovirt.org/#/c/39318/8/backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/vdscommands/RefreshDeviceSizeVDSCommandParameters.java File backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/vdscommands/RefreshDeviceSizeVDSCommandParameters.java: Line 12: this.deviceId = deviceId; Line 13: this.storageDomainId = storageDomainId; Line 14: } Line 15: Line 16: public RefreshDeviceSizeVDSCommandParameters() { This can be private (since it can't be removed) Line 17: } Line 18: Line 19: public String getDeviceId() { Line 20: return deviceId; Line 24: return storageDomainId; Line 25: } Line 26: Line 27: @Override Line 28: public String toString() { Let's use the "protected ToStringBuilder appendAttributes(ToStringBuilder tsb)" convention instead, which I think is available from VdsIdVDSCommandParametersBase Line 29: return String.format("%s,storageDomainId = %s deviceId=%s", Line 30: super.toString(), Line 31: getStorageDomainId(), Line 32: getDeviceId()); https://gerrit.ovirt.org/#/c/39318/8/backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/vdscommands/ResizeStorageDomainPVVDSCommandParameters.java File backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/vdscommands/ResizeStorageDomainPVVDSCommandParameters.java: Line 12: setStorageDomainId(storageDomainId); Line 13: this.device = device; Line 14: } Line 15: Line 16: public ResizeStorageDomainPVVDSCommandParameters() { Similar to other Parameters, private? Line 17: } Line 18: Line 19: public String getDevice() { Line 20: return device; Line 20: return device; Line 21: } Line 22: Line 23: @Override Line 24: public String toString() { Unfortunately I don't know if ToStringBuilder can be used here yet Line 25: return String.format("%s, device = %s", super.toString(), getDevice()); Line 26: } https://gerrit.ovirt.org/#/c/39318/8/backend/manager/modules/dal/src/main/resources/bundles/ExecutionMessages.properties File backend/manager/modules/dal/src/main/resources/bundles/ExecutionMessages.properties: Line 63: job.DeactivateStorageDomain=Deactivating Storage Domain ${Storage} in Data Center ${StoragePool} Line 64: job.DeactivateStorageDomainWithOvfUpdate=Deactivating Storage Domain ${Storage} in Data Center ${StoragePool}. Line 65: job.AddSANStorageDomain=Adding SAN Storage Domain ${Storage} Line 66: job.ExtendSANStorageDomain=Extending SAN Storage Domain ${Storage} Line 67: job.RefreshLunsSize=Refresh LUNs Size in Domain ${Storage} For clarity (though technically more than 1 LUN), let's go with "Refreshing LUN size in Storage Domain ${Storage} Line 68: job.RecoveryStoragePool=Recovering Data Center ${StoragePool} Line 69: job.RemoveStoragePool=Removing Data Center ${StoragePool} Line 70: job.UpdateStoragePool=Editing Data Center ${StoragePool} properties Line 71: job.AddExistingFileStorageDomain=Adding File Storage Domain ${Storage} https://gerrit.ovirt.org/#/c/39318/8/backend/manager/modules/dal/src/main/resources/bundles/VdsmErrors.properties File backend/manager/modules/dal/src/main/resources/bundles/VdsmErrors.properties: Line 324: ResourceDoesNotExist=Resource does not exist Line 325: InvalidResourceName=Invalid resource name Line 326: VolumeNotSparse=Volume type is not sparse Line 327: CannotSparsifyVolume=Cannot run virt-sparsify on volume Line 328: CouldNotResizePhysicalVolume=Could Not Resize Physical Volume No need to capitalize all words, eg "Could not resize Physical Volume" Line 329: # Gluster VDSM errors Line 330: GlusterGeneralException=Gluster General Exception Line 331: GlusterPermissionDeniedException=Permission denied Line 332: GlusterSyntaxErrorException=Syntax error -- 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: Greg Padgett <gpadg...@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
