Freddy Rolland has posted comments on this change. Change subject: engine: Add support for Refresh LUNs size ......................................................................
Patch Set 11: (15 comments) I did not respond to all comments, I will do as a later refactor https://gerrit.ovirt.org/#/c/39318/11/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 40: ; > you can remove the '=false' Well I did at first but Allon told me to assign for readability Line 78: ); > it's kind of confusing to have a set named with "List", consider change tha Done Line 85: } > you can replace the for loop with lunsList.removeAll(lunsFromDb) Tried it , did not work. I will leave it like this for now Line 91: ; > no need to init it here. Done Line 160: vdsList.add(vdsSizePair.getFirst()); Line 161: if (size == -1) { Line 162: size = vdsSizePair.getSecond(); Line 163: } else { Line 164: if (!size.equals(vdsSizePair.getSecond())) { > /s/else/elseif Done Line 165: failed = true; Line 166: } Line 167: } Line 168: } Line 161: if (size == -1) { Line 162: size = vdsSizePair.getSecond(); Line 163: } else { Line 164: if (!size.equals(vdsSizePair.getSecond())) { Line 165: failed = true; > why not break here? I want to have a list of all the failing VDS Line 166: } Line 167: } Line 168: } Line 169: if (failed) { Line 166: } Line 167: } Line 168: } Line 169: if (failed) { Line 170: failedVds.put(entry.getKey(), vdsList); > so if there is any failure we will always initiate this with the full list Yes Line 171: } Line 172: } Line 173: return failedVds; Line 174: } Line 179: log.debug("PV size after resize of LUN " + lun + " :" + pvSizeInBytes + " bytes"); Line 180: } Line 181: } Line 182: Line 183: private Long resizePV(String lunId) { > You already have a method with the same name "resizePV" consider to use a d Done Line 184: return (Long) runVdsCommand( Line 185: VDSCommandType.ResizeStorageDomainPV, Line 186: new ResizeStorageDomainPVVDSCommandParameters(getStoragePoolId(), Line 187: getStorageDomainId(), lunId)).getReturnValue(); Line 197: ()); > please rename - this host isn't neccessarily the spm. Done https://gerrit.ovirt.org/#/c/39318/11/backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/action/VdcActionType.java File backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/action/VdcActionType.java: Line 249: ConnectDomainToStorage(912, QuotaDependency.NONE), Line 250: DeactivateStorageDomain(909, ActionGroup.MANIPULATE_STORAGE_DOMAIN, QuotaDependency.NONE), Line 251: AddSANStorageDomain(910, ActionGroup.CREATE_STORAGE_DOMAIN, QuotaDependency.NONE), Line 252: ExtendSANStorageDomain(911, ActionGroup.EDIT_STORAGE_DOMAIN_CONFIGURATION, QuotaDependency.NONE), Line 253: RefreshLunsSize(917, ActionGroup.EDIT_STORAGE_DOMAIN_CONFIGURATION, QuotaDependency.NONE), > Consider to add this after RecoveryStoragePool (915) Done Line 254: ReconstructMasterDomain(913, QuotaDependency.NONE), Line 255: DeactivateStorageDomainWithOvfUpdate(914, ActionGroup.MANIPULATE_STORAGE_DOMAIN, QuotaDependency.NONE), Line 256: RecoveryStoragePool(915, ActionGroup.CREATE_STORAGE_POOL, QuotaDependency.NONE), Line 257: AddEmptyStoragePool(950, ActionGroup.CREATE_STORAGE_POOL, false, QuotaDependency.NONE), https://gerrit.ovirt.org/#/c/39318/11/backend/manager/modules/restapi/jaxrs/src/test/java/org/ovirt/engine/api/restapi/resource/BackendStorageDomainResourceTest.java File backend/manager/modules/restapi/jaxrs/src/test/java/org/ovirt/engine/api/restapi/resource/BackendStorageDomainResourceTest.java: Line 115: return entity; Line 116: } Line 117: Line 118: private org.ovirt.engine.core.common.businessentities.StorageDomain getIscsiEntity() { Line 119: org.ovirt.engine.core.common.businessentities.StorageDomain entity = control.createMock(org.ovirt.engine.core.common.businessentities.StorageDomain.class); > formatter? Done Line 120: expect(entity.getId()).andReturn(GUIDS[0]).anyTimes(); Line 121: expect(entity.getStorageName()).andReturn(NAMES[0]).anyTimes(); Line 122: expect(entity.getStorageDomainType()).andReturn(org.ovirt.engine.core.common.businessentities.StorageDomainType.Data).anyTimes(); Line 123: expect(entity.getStorageType()).andReturn(org.ovirt.engine.core.common.businessentities.storage.StorageType.ISCSI).anyTimes(); https://gerrit.ovirt.org/#/c/39318/11/backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/jsonrpc/JsonRpcVdsServer.java File backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/jsonrpc/JsonRpcVdsServer.java: Line 1228: public StatusOnlyReturnForXmlRpc glusterVolumeGeoRepSessionCreate(String volumeName, String remoteHost, String remotVolumeName, String remoteUserName, Boolean force) { Line 1229: JsonRpcRequest request = Line 1230: new RequestBuilder("GlusterVolume.geoRepSessionCreate").withParameter("volumeName", volumeName) Line 1231: .withParameter("remoteHost", remoteHost) Line 1232: .withParameter("remoteVolumeName", remotVolumeName) > Is this formatted? Done Line 1233: .withParameter("force", force) Line 1234: .withOptionalParameter("remoteUserName", remoteUserName).build(); Line 1235: Map<String, Object> response = new FutureMap(this.client, request); Line 1236: return new StatusOnlyReturnForXmlRpc(response); Line 1230: new RequestBuilder("GlusterVolume.geoRepSessionCreate").withParameter("volumeName", volumeName) Line 1231: .withParameter("remoteHost", remoteHost) Line 1232: .withParameter("remoteVolumeName", remotVolumeName) Line 1233: .withParameter("force", force) Line 1234: .withOptionalParameter("remoteUserName", remoteUserName).build(); > same Done Line 1235: Map<String, Object> response = new FutureMap(this.client, request); Line 1236: return new StatusOnlyReturnForXmlRpc(response); Line 1237: } Line 1238: Line 1396: JsonRpcRequest request = new RequestBuilder("GlusterVolume.geoRepConfigReset") Line 1397: .withParameter("volumeName", volumeName) Line 1398: .withParameter("remoteHost", slaveHost) Line 1399: .withParameter("remoteVolumeName", slaveVolumeName) Line 1400: .withParameter("optionName", configKey) > formatter? Done Line 1401: .build(); Line 1402: Map<String, Object> response = new FutureMap(this.client, request); Line 1403: return new StatusOnlyReturnForXmlRpc(response); Line 1404: } Line 1407: public GlusterVolumeGeoRepConfigListXmlRpc glusterVolumeGeoRepConfigList(String volumeName, String slaveHost, String slaveVolumeName) { Line 1408: JsonRpcRequest request = new RequestBuilder("GlusterVolume.geoRepConfigList") Line 1409: .withParameter("volumeName", volumeName) Line 1410: .withParameter("remoteHost", slaveHost) Line 1411: .withParameter("remoteVolumeName", slaveVolumeName) > formatter? Done Line 1412: .build(); Line 1413: Map<String, Object> response = new FutureMap(this.client, request); Line 1414: return new GlusterVolumeGeoRepConfigListXmlRpc(response); Line 1415: } -- 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: 11 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: Jenkins CI 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-HasComments: Yes _______________________________________________ Engine-patches mailing list Engine-patches@ovirt.org http://lists.ovirt.org/mailman/listinfo/engine-patches
