Maor Lipchuk has posted comments on this change. Change subject: core: prevent maintenance when domain is still in use ......................................................................
Patch Set 13: (7 comments) Generally the logic seems fine, few minor/cosmetic comments http://gerrit.ovirt.org/#/c/22045/13/backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/businessentities/StorageDomainStatus.java File backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/businessentities/StorageDomainStatus.java: Line 17: public static StorageDomainStatus forValue(int value) { Line 18: return values()[value]; Line 19: } Line 20: Line 21: public boolean isLocked() { I think that the method name is a bit misleading. Suggestion names could be: isLockedOrPreparingForMaintenance isStorageDomainInProcess Line 22: return this == Locked || this == PreparingForMaintenance; Line 23: } http://gerrit.ovirt.org/#/c/22045/13/backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/irsbroker/IrsBrokerCommand.java File backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/irsbroker/IrsBrokerCommand.java: Line 1042: mCurrentVdsId = null; Line 1043: } Line 1044: Line 1045: private final Map<Guid, HashSet<Guid>> _domainsInProblem = new ConcurrentHashMap<Guid, HashSet<Guid>>(); Line 1046: private final Map<Guid, HashSet<Guid>> _domainsInMaintenance = new ConcurrentHashMap<Guid, HashSet<Guid>>(); Better to use a java convention name, without the underline as a prefix: /s/_domainsInMaintenance/domainsInMaintenanceMap Line 1047: private final Map<Guid, String> _timers = new HashMap<Guid, String>(); Line 1048: Line 1049: public void updateVdsDomainsData(final Guid vdsId, final String vdsName, Line 1050: final ArrayList<VDSDomainsData> data) { Line 1214: } Line 1215: Line 1216: private void updateMaintenanceVdsData(final Guid vdsId, final String vdsName, Set<Guid> domainsInMaintenance) { Line 1217: for (Guid domainId : domainsInMaintenance) { Line 1218: Set<Guid> vdsSet = _domainsInMaintenance.get(domainId); Consider to use MultiValueMapUtils here, as so: MultiValueMapUtils.addToMap(domainId, vdsId, _domainsInMaintenance) (probably will work if the value type will be List instead of Set) Line 1219: if (vdsSet == null) { Line 1220: _domainsInMaintenance.put(domainId, new HashSet<>(Arrays.asList(vdsId))); Line 1221: } else { Line 1222: vdsSet.add(vdsId); Line 1216: private void updateMaintenanceVdsData(final Guid vdsId, final String vdsName, Set<Guid> domainsInMaintenance) { Line 1217: for (Guid domainId : domainsInMaintenance) { Line 1218: Set<Guid> vdsSet = _domainsInMaintenance.get(domainId); Line 1219: if (vdsSet == null) { Line 1220: _domainsInMaintenance.put(domainId, new HashSet<>(Arrays.asList(vdsId))); Consider adding a log that a domain was added Line 1221: } else { Line 1222: vdsSet.add(vdsId); Line 1223: } Line 1224: } Line 1224: } Line 1225: Set<Guid> maintenanceDomainsByHost = new HashSet<>(_domainsInMaintenance.keySet()); Line 1226: maintenanceDomainsByHost.removeAll(domainsInMaintenance); Line 1227: for (Guid domainId : maintenanceDomainsByHost) { Line 1228: Set<Guid> vdsForDomain = _domainsInMaintenance.get(domainId); Consider to use MultiValueMapUtils here, as so: MultiValueMapUtils.removeFromMap(domainId, vdsId, _domainsInMaintenance) (probably will work if the value type will be List instead of Set) Line 1229: if (vdsForDomain != null && vdsForDomain.contains(vdsId)) { Line 1230: vdsForDomain.remove(vdsId); Line 1231: if (vdsForDomain.isEmpty()) { Line 1232: _domainsInMaintenance.remove(domainId); Line 1228: Set<Guid> vdsForDomain = _domainsInMaintenance.get(domainId); Line 1229: if (vdsForDomain != null && vdsForDomain.contains(vdsId)) { Line 1230: vdsForDomain.remove(vdsId); Line 1231: if (vdsForDomain.isEmpty()) { Line 1232: _domainsInMaintenance.remove(domainId); Maybe it is worth to add a log here indicating that the domain is no longer in the map any more, just in case we will encounter some bugs of cleaning the Storage Domains from the cache Line 1233: } Line 1234: } Line 1235: } Line 1236: } Line 1456: Line 1457: } Line 1458: } Line 1459: Line 1460: private void removeVdsFromDomainMaintenance(List<Guid> nonOpVdss) { Consider to add a log here, that we clear the cache Line 1461: Iterator<Map.Entry<Guid, HashSet<Guid>>> iterDomainsInProblem = _domainsInMaintenance.entrySet().iterator(); Line 1462: while (iterDomainsInProblem.hasNext()) { Line 1463: Map.Entry<Guid, HashSet<Guid>> entry = iterDomainsInProblem.next(); Line 1464: entry.getValue().removeAll(nonOpVdss); -- To view, visit http://gerrit.ovirt.org/22045 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I55cd5aa6a6dc32f374a4bb21b159d3cb30da65f5 Gerrit-PatchSet: 13 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Federico Simoncelli <fsimo...@redhat.com> Gerrit-Reviewer: Allon Mureinik <amure...@redhat.com> Gerrit-Reviewer: Ayal Baron <aba...@redhat.com> Gerrit-Reviewer: Daniel Erez <de...@redhat.com> Gerrit-Reviewer: Federico Simoncelli <fsimo...@redhat.com> Gerrit-Reviewer: Juan Hernandez <juan.hernan...@redhat.com> Gerrit-Reviewer: Liron Ar <lara...@redhat.com> Gerrit-Reviewer: Maor Lipchuk <mlipc...@redhat.com> Gerrit-Reviewer: Sergey Gotliv <sgot...@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
