Liron Ar has posted comments on this change. Change subject: vdsbroker: improve the domain visibility cache ......................................................................
Patch Set 3: (10 comments) http://gerrit.ovirt.org/#/c/23370/3/backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/irsbroker/DomainVisibility.java File backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/irsbroker/DomainVisibility.java: Line 36: public DomainVisibility() { Line 37: this.domainsVisibility = new HashMap<>(); Line 38: } Line 39: Line 40: public void clear() { this should be synchronized as well Line 41: this.domainsVisibility.clear(); Line 42: } Line 43: Line 44: public synchronized void updatePoolDomains(Set<Guid> domains) { Line 50: domainsVisibility.keySet().retainAll(domains); Line 51: } Line 52: Line 53: public synchronized void setVdsDomainStatus(Guid vdsId, Guid domainId, Status status) { Line 54: Map<Guid, Status> domainVdsStatus = domainsVisibility.get(domainId); what about null recieved here? Line 55: Line 56: if (status == Status.ACTIVE || status == Status.UNKNOWN) { Line 57: domainVdsStatus.remove(vdsId); Line 58: } else { Line 66: } Line 67: } Line 68: Line 69: public synchronized HashSet<Guid> getDomainProblematicVdsList(Guid domainId) { Line 70: HashSet<Guid> vdsList = new HashSet<>(); please rename, that a set named as list :) Line 71: Map<Guid, Status> domainVdsStatus = domainsVisibility.get(domainId); Line 72: Line 73: for (Entry<Guid, Status> status : domainVdsStatus.entrySet()) { Line 74: if (status.getValue().isProblematic()) { Line 67: } Line 68: Line 69: public synchronized HashSet<Guid> getDomainProblematicVdsList(Guid domainId) { Line 70: HashSet<Guid> vdsList = new HashSet<>(); Line 71: Map<Guid, Status> domainVdsStatus = domainsVisibility.get(domainId); how about using ConcurrentHashMap so which will take some care of concurrency? we don't need to block on that for example. Line 72: Line 73: for (Entry<Guid, Status> status : domainVdsStatus.entrySet()) { Line 74: if (status.getValue().isProblematic()) { Line 75: vdsList.add(status.getKey()); http://gerrit.ovirt.org/#/c/23370/3/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 1015: return poolDomainMap; Line 1016: } Line 1017: Line 1018: public void updateDomainsVisibility(VDS vds, ArrayList<VDSDomainsData> domainsDataList) { Line 1019: // Step 1: return if there's no data to analyze and remove the vds if it is in Maintenance. what about the pool status check that was here before? Line 1020: // It's not correct to remove the NonOperational hosts because they may be still connected Line 1021: // to the pool. They must be moved to Maintenance. Line 1022: if (domainsDataList == null) { Line 1023: if (vds.getStatus() == VDSStatus.Maintenance) { Line 1019: // Step 1: return if there's no data to analyze and remove the vds if it is in Maintenance. Line 1020: // It's not correct to remove the NonOperational hosts because they may be still connected Line 1021: // to the pool. They must be moved to Maintenance. Line 1022: if (domainsDataList == null) { Line 1023: if (vds.getStatus() == VDSStatus.Maintenance) { why do we even get here with host in maintenance? Line 1024: domainsVisibility.removeVdsStatuses(vds.getId()); Line 1025: } Line 1026: return; Line 1027: } Line 1056: vds.getId(), poolDomain.getId(), domainStatus.toString()); Line 1057: } Line 1058: Line 1059: domainsVisibility.setVdsDomainStatus(vds.getId(), poolDomain.getId(), domainStatus); Line 1060: why do we keep also the domains that are fine? in 99% of the cases all hosts will see all the domains as fine and we will need to maintain this possibly big structure, whats the motivation for not keep there only the problematic data? Line 1061: if (domainStatus == DomainVisibility.Status.UNREPORTED Line 1062: || domainStatus == DomainVisibility.Status.UNREACHABLE) { Line 1063: setTimerIfAbsent(poolDomain.getId()); Line 1064: } else if (domainsVisibility.getDomainProblematicVdsList(poolDomain.getId()).size() == 0) { Line 1149: // build a list of all the hosts that did not report Line 1150: // on this domain as in problem. Line 1151: // Mark the above list as hosts we suspect are in Line 1152: // problem. Line 1153: Set<Guid> hostsThatReportedDomainAsInProblem = domainsVisibility.getDomainProblematicVdsList(domainId); this set can be passed on the call if we already got it in line 1215 Line 1154: List<Guid> vdssInProblem = new ArrayList<Guid>(); Line 1155: for (Guid tempVDSId : vdssInPool) { Line 1156: if (!hostsThatReportedDomainAsInProblem.contains(tempVDSId)) { Line 1157: vdssInProblem.add(tempVDSId); Line 1173: // reported as in problem. Line 1174: // Moving all the hosts which reported on Line 1175: // this domain as in problem to non Line 1176: // operational. Line 1177: for (final Guid vdsId : domainsVisibility.getDomainProblematicVdsList(domainId)) { or atleast be re used here. Line 1178: VDS vds = vdsMap.get(vdsId); Line 1179: if (vds == null) { Line 1180: log.warnFormat( Line 1181: "vds {0} reported domain {1} - as in problem but cannot find vds in db!!", Line 1236: return result; Line 1237: } Line 1238: Line 1239: private void setTimerIfAbsent(Guid domainId) { Line 1240: synchronized (_timers) { you add under this synchronized but not all modifications to this map are using it thus no garuanteed happens-before relationship. Line 1241: if (_timers.containsKey(domainId)) Line 1242: return; Line 1243: Line 1244: String jobId = SchedulerUtilQuartzImpl.getInstance().scheduleAOneTimeJob( -- To view, visit http://gerrit.ovirt.org/23370 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Id74f12d108f75924fbff28409a89bc1caff05908 Gerrit-PatchSet: 3 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Federico Simoncelli <[email protected]> Gerrit-Reviewer: Allon Mureinik <[email protected]> Gerrit-Reviewer: Ayal Baron <[email protected]> Gerrit-Reviewer: Daniel Erez <[email protected]> Gerrit-Reviewer: Federico Simoncelli <[email protected]> Gerrit-Reviewer: Liron Ar <[email protected]> Gerrit-Reviewer: Sergey Gotliv <[email protected]> Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes _______________________________________________ Engine-patches mailing list [email protected] http://lists.ovirt.org/mailman/listinfo/engine-patches
