Liron Ar has posted comments on this change. Change subject: core: change domains status to unknown when there are no reporting hosts ......................................................................
Patch Set 1: (8 comments) http://gerrit.ovirt.org/#/c/25542/1//COMMIT_MSG Commit Message: Line 8: Line 9: Currently the monitored pool domains 'change' their statuses according to Line 10: the reports received by the pool hosts that are in status UP. Line 11: Line 12: When there are no reporting hosts - sthe status of the pool domains that are > /s/sthe/the Done Line 13: monitored should be changed to unknown, because otherwise nothing will trigger Line 14: the status change. In that case (an example) we might be left with a domain Line 15: in 'active' status although there are no reporting hosts. Line 16: http://gerrit.ovirt.org/#/c/25542/1/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 86: import org.ovirt.engine.core.vdsbroker.xmlrpc.XmlRpcUtils; Line 87: @Logged(errorLevel = LogLevel.ERROR) Line 88: public abstract class IrsBrokerCommand<P extends IrsBaseVDSCommandParameters> extends BrokerCommandBase<P> { Line 89: private static Map<Guid, IrsProxyData> _irsProxyData = new ConcurrentHashMap<Guid, IrsProxyData>(); Line 90: private static final VDSStatus reportingVdsStatus = VDSStatus.Up; > This variable name make the process be more vague, the only changed thing is that the status is exported to a variable. this is done to use the same variable in lines 100 and 199 so there won't be a case in which someone changes this status in one of the places and not in the other one. the name suggests that this is the status in which we consider reports, if you have better name i'm open for suggestions. Line 91: Line 92: /** Line 93: * process received domain monitoring information from a given vds if necessary (according to it's status). Line 94: * @param vds Line 199: .getAllForStoragePoolAndStatus(_storagePoolId, reportingVdsStatus) Line 200: .isEmpty()) { Line 201: StoragePoolDomainHelper.updateApplicablePoolDomainsStatuses(_storagePoolId, Line 202: StoragePoolDomainHelper.storageDomainMonitoredStatus, Line 203: StorageDomainStatus.Unknown); > 1) Why changing the status of inactive Storage Domains to be unknown? If we 1. The monitored pool domains 'change' their statuses according to the reports received by the pool hosts that are in status UP - the status reflects the status of the domain as seen by the current activated hosts only. if you don't have any reporting hosts, you don't know the status of the connection between the hosts and the storage. 2. DOne Line 204: } Line 205: Line 206: if (storagePool.getStatus() == StoragePoolStatus.Up Line 207: || storagePool.getStatus() == StoragePoolStatus.NonResponsive || storagePool http://gerrit.ovirt.org/#/c/25542/1/backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/storage/StoragePoolDomainHelper.java File backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/storage/StoragePoolDomainHelper.java: Line 1: package org.ovirt.engine.core.vdsbroker.storage; > this package is not be related to the class? what was it moved from utils? 1. it was moved from utils to use the db facade. 2. why the package isn't relevant? it's a helper used solely from vds commands. Line 2: Line 3: import java.util.Collections; Line 4: import java.util.HashMap; Line 5: import java.util.HashSet; Line 14: Line 15: Line 16: public class StoragePoolDomainHelper { Line 17: Line 18: public static final Set<StorageDomainStatus> storageDomainMonitoredStatus; > /s/ storageDomainMonitoredStatus/storageDomainMonitoredStatuses Done Line 19: Line 20: static { Line 21: HashSet<StorageDomainStatus> storageDomainStatuses = new HashSet<>(); Line 22: storageDomainStatuses.add(StorageDomainStatus.InActive); Line 16: public class StoragePoolDomainHelper { Line 17: Line 18: public static final Set<StorageDomainStatus> storageDomainMonitoredStatus; Line 19: Line 20: static { > why not extract this to a static method? and let it be called when declarin what's the benefit that you see comparing to the current implmenetation? please elaborate. Line 21: HashSet<StorageDomainStatus> storageDomainStatuses = new HashSet<>(); Line 22: storageDomainStatuses.add(StorageDomainStatus.InActive); Line 23: storageDomainStatuses.add(StorageDomainStatus.Active); Line 24: storageDomainMonitoredStatus = Collections.unmodifiableSet(storageDomainStatuses); Line 39: return storageDomains; Line 40: } Line 41: Line 42: public static void updateApplicablePoolDomainsStatuses(Guid storagePoolId, Line 43: Set<StorageDomainStatus> applicableStatusesForUpdate, > Suggestion: 1. it's intenionaly not private, as it's being used from the outside. this class is a helper, so i use it also for holding the constant which makes sense IMO. 2. i prefer not to, this is helper class - and imo this logic is too "BLLish". if we'll see that it's commonly used with those parameters, we can always add such method. Line 44: StorageDomainStatus newStatus) { Line 45: List<StoragePoolIsoMap> storagesStatusInPool = DbFacade.getInstance() Line 46: .getStoragePoolIsoMapDao().getAllForStoragePool(storagePoolId); Line 47: for (StoragePoolIsoMap storageStatusInPool : storagesStatusInPool) { http://gerrit.ovirt.org/#/c/25542/1/backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/vdsbroker/ReconstructMasterVDSCommand.java File backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/vdsbroker/ReconstructMasterVDSCommand.java: Line 4: Line 5: import org.ovirt.engine.core.common.config.Config; Line 6: import org.ovirt.engine.core.common.config.ConfigValues; Line 7: import org.ovirt.engine.core.common.vdscommands.ReconstructMasterVDSCommandParameters; Line 8: import org.ovirt.engine.core.vdsbroker.storage.StoragePoolDomainHelper; > not relevant to the patch it is relevant, as i moved this helper a package because of the changes in this patch so the import has changed. Line 9: Line 10: public class ReconstructMasterVDSCommand<P extends ReconstructMasterVDSCommandParameters> extends VdsBrokerCommand<P> { Line 11: public ReconstructMasterVDSCommand(P parameters) { Line 12: super(parameters); -- To view, visit http://gerrit.ovirt.org/25542 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I8091a4864711aeccee41effb1bc7d9823a1870c7 Gerrit-PatchSet: 1 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Liron Ar <[email protected]> Gerrit-Reviewer: Allon Mureinik <[email protected]> Gerrit-Reviewer: Daniel Erez <[email protected]> Gerrit-Reviewer: Federico Simoncelli <[email protected]> Gerrit-Reviewer: Liron Ar <[email protected]> Gerrit-Reviewer: Maor Lipchuk <[email protected]> Gerrit-Reviewer: Tal Nisan <[email protected]> Gerrit-Reviewer: [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
