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: (5 comments) 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; > the only changed thing is that the status is exported to a variable. I would not use a variable at all, but that's only my opinion. not crucial. 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. The monitored pool domains 'change' their statuses according to but if the domain is already in status inactive, then we do know that once, hosts did reported it to be inactive, so we do have an indication that the storage domain had a problem. What will be the benefit of changing the inactive sttus to unknown? I just think of the following scenario: User had a Data Center, with a domain and few hosts, the domain got corrupted, so it became inactive. The user removed all the hosts from the Data Center and started to work on a new one, so now the Storage Domain will become unknown, Later on the user might be confused and try to re-active this old Data Center, since we will not leave any indication that this Storage has a problem 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; > 1. it was moved from utils to use the db facade. based on the use of the db facade I agree. Line 2: Line 3: import java.util.Collections; Line 4: import java.util.HashMap; Line 5: import java.util.HashSet; Line 16: public class StoragePoolDomainHelper { Line 17: Line 18: public static final Set<StorageDomainStatus> storageDomainMonitoredStatus; Line 19: Line 20: static { > what's the benefit that you see comparing to the current implmenetation? pl moving it to a method will make it more clean and testable 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); 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; > it is relevant, as i moved this helper a package because of the changes in yes, I see it now. 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
