Maor Lipchuk has posted comments on this change. Change subject: core: determine wether domain monitoring result is actual ......................................................................
Patch Set 2: (5 comments) https://gerrit.ovirt.org/#/c/38039/2//COMMIT_MSG Commit Message: Line 7: determine /s/wether/whether https://gerrit.ovirt.org/#/c/38039/2/backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/FeatureSupported.java File backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/FeatureSupported.java: Line 479: reportWhetherDomainMonitoringResultIsActual This is a boolean method, it does not report anything please change the name to isActualResultForDomainMonitorSupported, it is much more shorter and informative https://gerrit.ovirt.org/#/c/38039/2/backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/config/ConfigValues.java File backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/config/ConfigValues.java: Line 2040: ReportWhetherDomainMonitoringResultIsActual Please add Supported or Enabled at the end of the name (unification reasons). Also, consider to change the name to ActualResultForDomainMonitorSupported https://gerrit.ovirt.org/#/c/38039/2/backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/irsbroker/IrsProxyData.java File backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/irsbroker/IrsProxyData.java: Line 1232: boolean same question as Allon asked, I'm also didn't follow what will happened if the actual will be equals to NOT_ACTUAL, it appears that valid() and invalid() will be false, no? also, please use formatter Line 1234: Line 1235: private DomainMonitoringResult analyzeDomainReport(VDSDomainsData tempData, StoragePool storagePool, boolean isLog) { Line 1236: if (!tempData.isActual() && Line 1237: FeatureSupported.reportWhetherDomainMonitoringResultIsActual(storagePool.getCompatibilityVersion())) { Line 1238: log.error("Domain '{}' report isn't an actual report", > I'd log.warn here - this isn't an error per-se +1, I would also add more information about it, since "actual" is not that informative. Probably something like "... Domain monitor has not yet monitored Domain '{}'" Line 1239: getDomainIdTuple(tempData.getDomainId())); Line 1240: return DomainMonitoringResult.NOT_ACTUAL; Line 1241: } Line 1242: -- To view, visit https://gerrit.ovirt.org/38039 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Iddbba94d81694bd6cb1ce3648872eb81182a50ab Gerrit-PatchSet: 2 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Liron Aravot <lara...@redhat.com> Gerrit-Reviewer: Allon Mureinik <amure...@redhat.com> Gerrit-Reviewer: Maor Lipchuk <mlipc...@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
