Freddy Rolland has posted comments on this change. Change subject: engine: Add support for Refresh LUNs size ......................................................................
Patch Set 10: (13 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. Done 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 Done 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 Done 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. Done 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 "/*" Done 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/ Done 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 Done 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? I want to have a list of all hosts that have issues for the log. 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. Done 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 Done 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? Done 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? Done 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!) Done Line 58: Line 59: @Before Line 60: public void setUp() { Line 61: sdId = Guid.newGuid(); -- 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
