Allon Mureinik has posted comments on this change. Change subject: engine: Add support for Refresh LUNs size ......................................................................
Patch Set 10: Code-Review-1 (20 comments) https://gerrit.ovirt.org/#/c/39318/10/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 34: @NonTransactiveCommandAttribute(forceCompensation = true) Line 35: public class RefreshLunsSizeCommand<T extends ExtendSANStorageDomainParameters> extends Line 36: StorageDomainCommandBase<T> { Line 37: Line 38: private boolean device_size_visibility_error; thisIsJavaCode, it_is_not_c_or_python. Also, worth explicitly initializing to false, for readability reasons. Line 39: Line 40: public RefreshLunsSizeCommand(T parameters, CommandContext commandContext) { Line 41: super(parameters, commandContext); Line 42: } Line 72: Line 73: private boolean checkLunsInStorageDomain(List<String> lunIds, StorageDomain storageDomain) { Line 74: // Get LUNs from DB Line 75: List<LUNs> lunsFromDb = getLunDao().getAllForVolumeGroup(getStorageDomain().getStorage()); Line 76: List<String> lunsList = new ArrayList<>(lunIds); You don't care about the order, and you're doing several remove operations - better use a HashSet Line 77: Line 78: for (LUNs lun : lunsFromDb) { Line 79: if (lunsList.contains(lun.getLUN_id())) { Line 80: // LUN is part of the storage domain Line 104: String lunId = entry.getKey(); Line 105: List<VDS> vdsList = entry.getValue(); Line 106: log.error("Failed to refresh device " + lunId + " Not all VDS are seeing the same size " + Line 107: "VDS :" + vdsList); Line 108: String vdsListString = StringUtils.join(getVdsNameList(vdsList), ", "); //$NON-NLS-1$ this is backend code, no need for the NON-NLS comment Line 109: failedVds.add("LUN : " + lunId + "VDS: " + vdsListString); Line 110: } Line 111: Line 112: throw new VdcBLLException(VdcBllErrors.REFRESH_LUN_ERROR, Line 124: Line 125: setSucceeded(true); Line 126: } Line 127: Line 128: private List getVdsNameList(List<VDS> vdsList) { This method essentially is Entities.objectNames - please use that instead. Line 129: List<String> nameList = new ArrayList<>(); Line 130: for (VDS vds : vdsList) { Line 131: nameList.add(vds.getName()); Line 132: } Line 132: } Line 133: return nameList; Line 134: } Line 135: Line 136: /* javadoc: should start with "/**", not "/*" Line 137: This method is calling GetDeviceList with the specified luns on all hosts. Line 138: In VDSM , this call will resize the devices if needed. Line 139: It returns a map of LUN ID to a list of Pair(VDS,Size) Line 140: This map will help to check if all hosts are seeing the same size of the LUNs. Line 133: return nameList; Line 134: } Line 135: Line 136: /* Line 137: This method is calling GetDeviceList with the specified luns on all hosts. s/is calling/calls/ Line 138: In VDSM , this call will resize the devices if needed. Line 139: It returns a map of LUN ID to a list of Pair(VDS,Size) Line 140: This map will help to check if all hosts are seeing the same size of the LUNs. Line 141: */ Line 153: if (vdsSizePairs == null) { Line 154: vdsSizePairs = new ArrayList<>(); Line 155: lunToVds.put(lun.getLUN_id(), vdsSizePairs); Line 156: } Line 157: vdsSizePairs.add(new Pair<VDS, Integer>(vds, lun.getDeviceSize())); Take a look at MultiValueMapUtils - it will save you this if statement Line 158: } Line 159: } Line 160: return lunToVds; Line 161: } Line 171: if (size == -1) { Line 172: size = vdsSizePair.getSecond(); Line 173: } else { Line 174: if (!size.equals(vdsSizePair.getSecond())) { Line 175: failed = true; Wouldn't breaking after finding failed=true save some iterations? Line 176: } Line 177: } Line 178: } Line 179: if (failed) { Line 210: return vds.getSpmStatus() == VdsSpmStatus.SPM; Line 211: } Line 212: })); Line 213: return runVdsCommand(VDSCommandType.GetStorageDomainStats, Line 214: new GetStorageDomainStatsVDSCommandParameters(spmVds.getId(), getParameters().getStorageDomainId())); In 3.6.0 we will not have an SPM. In this case, just take the first host. Line 215: } Line 216: Line 217: protected void updateStorageDomain(final StorageDomain storageDomainToUpdate) { Line 218: executeInNewTransaction(new TransactionMethod<Void>() { https://gerrit.ovirt.org/#/c/39318/10/backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/storage/RefreshLunsSizeCommandTest.java File backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/storage/RefreshLunsSizeCommandTest.java: Line 36: Line 37: @RunWith(MockitoJUnitRunner.class) Line 38: public class RefreshLunsSizeCommandTest { Line 39: Line 40: static String STORAGE = "STORAGE"; it's a constant - should be private static final Line 41: private Guid sdId; Line 42: private StorageDomain sd; Line 43: private StoragePool sp; Line 44: private StorageDomainStatic sdStatic; Line 43: private StoragePool sp; Line 44: private StorageDomainStatic sdStatic; Line 45: private Guid spId; Line 46: private RefreshLunsSizeCommand<ExtendSANStorageDomainParameters> cmd; Line 47: ArrayList<String> lunIdList; Why an ArrayList and not a List? Why not private? Line 48: List<LUNs> lunsFromDb; Line 49: Line 50: @Mock Line 51: private StorageDomainStaticDAO sdsDao; Line 44: private StorageDomainStatic sdStatic; Line 45: private Guid spId; Line 46: private RefreshLunsSizeCommand<ExtendSANStorageDomainParameters> cmd; Line 47: ArrayList<String> lunIdList; Line 48: List<LUNs> lunsFromDb; Why not private? Line 49: Line 50: @Mock Line 51: private StorageDomainStaticDAO sdsDao; Line 52: Line 53: @Mock Line 54: private LunDAO lunsDao; Line 55: Line 56: @Mock Line 57: private IConfigUtilsInterface configUtils; Just use MockConfigRule - so much easier (and cleaner!) Line 58: Line 59: @Before Line 60: public void setUp() { Line 61: sdId = Guid.newGuid(); https://gerrit.ovirt.org/#/c/39318/10/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 747: } Line 748: Line 749: @Override Line 750: public LUNListReturnForXmlRpc getDeviceList(int storageType, String[] devicesList) { Line 751: ArrayList<String> devicesListArray = should be defined as List, no? Line 752: devicesList != null ? new ArrayList<String>(Arrays.asList(devicesList)) : null; Line 753: JsonRpcRequest request = Line 754: new RequestBuilder("Host.getDeviceList").withParameter("storageType", Line 755: storageType).withOptionalParameterAsList("guids", devicesListArray).build(); 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(); Unrelared to the patch, please remove Line 1235: Map<String, Object> response = new FutureMap(this.client, request); Line 1236: return new StatusOnlyReturnForXmlRpc(response); Line 1237: } Line 1238: Line 1289: public StatusOnlyReturnForXmlRpc glusterVolumeGeoRepSessionDelete(String volumeName, String remoteHost, String remoteVolumeName) { Line 1290: JsonRpcRequest request = new RequestBuilder("GlusterVolume.geoRepSessionDelete") Line 1291: .withParameter("volumeName", volumeName) Line 1292: .withParameter("remoteHost", remoteHost) Line 1293: .withParameter("remoteVolumeName", remoteVolumeName) Unrelared to the patch, please remove Line 1294: .build(); Line 1295: Map<String, Object> response = new FutureMap(this.client, request); Line 1296: return new StatusOnlyReturnForXmlRpc(response); Line 1297: } Line 1301: JsonRpcRequest request = new RequestBuilder("GlusterVolume.geoRepSessionStop") Line 1302: .withParameter("volumeName", volumeName) Line 1303: .withParameter("remoteHost", remoteHost) Line 1304: .withParameter("remoteVolumeName", remoteVolumeName) Line 1305: .withParameter("force", force) Unrelared to the patch, please remove Line 1306: .build(); Line 1307: Map<String, Object> response = new FutureMap(this.client, request); Line 1308: return new StatusOnlyReturnForXmlRpc(response); Line 1309: } 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) Unrelared to the patch, please remove 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) Unrelared to the patch, please remove Line 1412: .build(); Line 1413: Map<String, Object> response = new FutureMap(this.client, request); Line 1414: return new GlusterVolumeGeoRepConfigListXmlRpc(response); Line 1415: } https://gerrit.ovirt.org/#/c/39318/10/backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/vdsbroker/VdsServerWrapper.java File backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/vdsbroker/VdsServerWrapper.java: Line 671: @Override Line 672: public LUNListReturnForXmlRpc getDeviceList(int storageType, String[] devicesList) { Line 673: try { Line 674: Map<String, Object> xmlRpcReturnValue = null; Line 675: if(devicesList != null){ formatting: need spaces before "(", "{" and "}" Line 676: xmlRpcReturnValue = vdsServer.getDeviceList(storageType, devicesList); Line 677: }else{ Line 678: xmlRpcReturnValue = vdsServer.getDeviceList(storageType); Line 679: } -- 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: 10 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
