Freddy Rolland has posted comments on this change. Change subject: engine: Add support for Refresh LUNs size ......................................................................
Patch Set 5: (13 comments) https://gerrit.ovirt.org/#/c/39318/5/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 49: if (!(checkStorageDomain() && checkStorageDomainStatus(StorageDomainStatus.Active))) { Line 50: return false; Line 51: } Line 52: Line 53: if ((getStorageDomain().getStorageType() == StorageType.NFS || getStorageDomain().getStorageType() == StorageType.UNKNOWN)) { > This should be: Done Line 54: addCanDoActionMessage(VdcBllMessages.ACTION_TYPE_FAILED_STORAGE_DOMAIN_TYPE_ILLEGAL); Line 55: return false; Line 56: } Line 57: Line 76: Line 77: List<String> succeededRefreshLuns = new ArrayList<>(); Line 78: for (Map.Entry<String, Map<Long, List<VDS>>> entry : lunToSize.entrySet()) { Line 79: if (entry.getValue().size() != 1) { Line 80: log.error("Failed to refresh device " + entry.getKey() + " Not all VDS are seeing the same size"); > We need more info in this error. We should know here the expected size repo Done Line 81: } else{ Line 82: String lunId = entry.getKey(); Line 83: // Call PV resize on SPM Line 84: Long pvSize = resizePV(lunId); Line 124: log.error("Failed to refresh PV " + lunId, e); Line 125: } Line 126: } Line 127: Line 128: private Map<String, Map<Long, List<VDS>>> refreshLunSizeAllVds(List<String> luns) { > This method should get the expected size of each lun, since vdsm reported i I have change the logic accordingly Line 129: Map<String, Map<Long, List<VDS>>> lunToSize = new HashMap<>(); Line 130: for (String lun : luns) { Line 131: Map<Long, List<VDS>> lunSizeToVDs = new HashMap<>(); Line 132: lunToSize.put(lun, lunSizeToVDs); Line 138: vdsForSize = new ArrayList<>(); Line 139: lunSizeToVDs.put(size, vdsForSize); Line 140: } Line 141: vdsForSize.add(vds); Line 142: } > This will not scale. You call a blocking method for each host and wait for Scale will not be handle at first stage Line 143: } Line 144: return lunToSize; Line 145: } Line 146: https://gerrit.ovirt.org/#/c/39318/5/backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/FeatureSupported.java File backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/FeatureSupported.java: Line 519: * @param version Line 520: * Compatibility version to check for. Line 521: * @return <code>true</code> if Refresh LUNs is supported for the given version Line 522: */ Line 523: public static boolean isRefreshLunsSupported(Version version) { > Other methods here do not use "is" prefix, and they look nicer. Done Line 524: return supportedInConfig(ConfigValues.RefreshLunsSupported, version); Line 525: } https://gerrit.ovirt.org/#/c/39318/5/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 11: this.deviceId = deviceId; Line 12: } Line 13: Line 14: public RefreshDeviceSizeVDSCommandParameters() { Line 15: } > Why do we need empty constructor? Being able to create an object without a It seems there is a check style error if I remove the empty constructor /home/frolland/git/ovirt-engine/backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/vdscommands/RefreshDeviceSizeVDSCommandParameters.java:5:14: Class RefreshDeviceSizeVDSCommandParameters must have a no-argument constructor (with any access modifier) Line 16: Line 17: public String getDeviceId() { Line 18: return deviceId; Line 19: } Line 17: public String getDeviceId() { Line 18: return deviceId; Line 19: } Line 20: Line 21: public void setDeviceId(String deviceId) { > Do we need to set this parameter? Why not use an immutable object? Done Line 22: this.deviceId = deviceId; Line 23: } Line 24: Line 25: @Override https://gerrit.ovirt.org/#/c/39318/5/backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/vdscommands/ResizePVStorageDomainVDSCommandParameters.java File backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/vdscommands/ResizePVStorageDomainVDSCommandParameters.java: Line 12: Line 13: public ResizePVStorageDomainVDSCommandParameters() { Line 14: } Line 15: Line 16: private String privateDevice; > Why define this after the constructors and not before them? And why is this Done Line 17: Line 18: public String getDevice() { Line 19: return privateDevice; Line 20: } Line 24: } Line 25: Line 26: @Override Line 27: public String toString() { Line 28: return String.format("%s, deviceList = %s", super.toString(), getDevice()); > getDevice returns a string, why "deviceList"? Done Line 29: } https://gerrit.ovirt.org/#/c/39318/5/backend/manager/modules/restapi/jaxrs/src/main/java/org/ovirt/engine/api/restapi/resource/BackendStorageDomainResource.java File backend/manager/modules/restapi/jaxrs/src/main/java/org/ovirt/engine/api/restapi/resource/BackendStorageDomainResource.java: Line 166: /** Line 167: * Resize the LUNs if the user specified Line 168: * @param incoming Line 169: * @param storageDomain Line 170: * @param storageType > This documentation does not add any value Done Line 171: */ Line 172: private void refreshLunSize(StorageDomain incoming, StorageDomain storageDomain, StorageType storageType) { Line 173: if (incoming.getStorage() == null) { Line 174: // LUNs info was not supplied in the request so no need to check whether to refresh the size Line 174: // LUNs info was not supplied in the request so no need to check whether to refresh the size Line 175: return; Line 176: } Line 177: List<LogicalUnit> incomingLuns = getIncomingLuns(incoming.getStorage()); Line 178: if (!incomingLuns.isEmpty()) { > We don't need this check. Better find the luns that need to be resized. If Done Line 179: List<LogicalUnit> lunsToResize = new ArrayList<>(); Line 180: for(LogicalUnit logicalUnit: incomingLuns){ Line 181: if(logicalUnit.isSetRefreshSize() && logicalUnit.isRefreshSize()){ Line 182: lunsToResize.add(logicalUnit); Line 177: List<LogicalUnit> incomingLuns = getIncomingLuns(incoming.getStorage()); Line 178: if (!incomingLuns.isEmpty()) { Line 179: List<LogicalUnit> lunsToResize = new ArrayList<>(); Line 180: for(LogicalUnit logicalUnit: incomingLuns){ Line 181: if(logicalUnit.isSetRefreshSize() && logicalUnit.isRefreshSize()){ > This is code not very clear. What is "isSetRefreshSize? What is "isRefreshS isSetXXXX - is a method from rest api to see if the user provided a value to this fields, otherwise it may be null Line 182: lunsToResize.add(logicalUnit); Line 183: } Line 184: } Line 185: if(!lunsToResize.isEmpty()){ https://gerrit.ovirt.org/#/c/39318/5/backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/jsonrpc/JsonRpcIIrsServer.java File backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/jsonrpc/JsonRpcIIrsServer.java: Line 367: return new StatusOnlyReturnForXmlRpc(response); Line 368: } Line 369: Line 370: @Override Line 371: public RefreshDeviceSizeMapReturnForXmlRpc resizePVStorageDomain(String sdUUID, String spUUID, String device) { > resizePVStorageDomain does not make sense. We cannot replace "extend" with Done Line 372: JsonRpcRequest request = Line 373: new RequestBuilder("StorageDomain.resizePV").withParameter("storagedomainID", sdUUID) Line 374: .withParameter("storagepoolID", spUUID) Line 375: .withParameter("guid", device) -- 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: 5 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: 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
