Maor Lipchuk 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 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, before, we knew that the host status is up, now it is more confusing IMHO. im my opinion it is better to relinquish the use of the new variable, or rename it to be clear the Host is Up. 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 know there is a problem in the Storage Domain why losing this data? 2) Please add a log of the storage domains' statuses being changed 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? 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 should be plural since it is a set 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 declaring storageDomainMonitoredStatus. as so : public static final Set<StorageDomainStatus> storageDomainMonitoredStatus = inistializeMonitoredStatuses(); 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) make storageDomainMonitoredStatus access modifier private 2) overload the method to be without the applicableStatusesForUpdate parameter, and use the variable internally, instead force the client to pass it as a variable to the method that already contain it. 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 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
