Martin Peřina has posted comments on this change. Change subject: core: FenceProxyLocator* code clean up ......................................................................
Patch Set 1: (3 comments) https://gerrit.ovirt.org/#/c/39763/1/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/pm/FenceProxyLocator.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/pm/FenceProxyLocator.java: Line 71: break; Line 72: } Line 73: } Line 74: if (proxyHost == null) { Line 75: log.error("Can not run fence action on host '{}', no suitable proxy host was found.", > we have a mixed terminology all around of "Power Management" and fence , II Well, AFAIK we use term "power management" in UI and audit log, but "fence" term is used in engine.log logging, because in the code we use "fence" term more often. But I agree with you, for 4.0 we should select one term and eliminate usage of the other one at all places. Line 76: fencedHost.getName()); Line 77: return null; Line 78: } Line 79: return proxyHost; Line 145: return proxyCandidate.getSupportedClusterVersionsSet().contains(minimalSupportedVersion); Line 146: } Line 147: Line 148: private boolean isHostNetworkUnreachable(VDS proxyCandidate) { Line 149: return proxyCandidate.getStatus() == VDSStatus.Down > why this code was changed? 1. There's no need to to save VDS.getDynamicData() into its own variable and call getStatus() on it when VDS object itself provide getStatus() method 2. Renaming vds to proxyCandidate better described what VDS instance is stored in the variable Line 150: || proxyCandidate.getStatus() == VDSStatus.Reboot Line 151: || proxyCandidate.getStatus() == VDSStatus.Kdumping Line 152: || proxyCandidate.getStatus() == VDSStatus.NonResponsive Line 153: || proxyCandidate.getStatus() == VDSStatus.PendingApproval https://gerrit.ovirt.org/#/c/39763/1/backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/pm/FenceProxyLocatorTest.java File backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/pm/FenceProxyLocatorTest.java: Line 13: import org.junit.Test; Line 14: import org.junit.runner.RunWith; Line 15: import org.mockito.Mock; Line 16: import org.mockito.runners.MockitoJUnitRunner; Line 17: import org.ovirt.engine.core.bll.DbDependentTestBase; > why do we need this, I see no usage of it in this code ... The test class extends DbDependentTestBase and when I moved the test class into pm subpackage, this import is required. And since we mock DBFacade in test class, we need to inherit from DbDependentTestBase. Line 18: import org.ovirt.engine.core.common.businessentities.FencingPolicy; Line 19: import org.ovirt.engine.core.common.businessentities.NonOperationalReason; Line 20: import org.ovirt.engine.core.common.businessentities.VDS; Line 21: import org.ovirt.engine.core.common.businessentities.VDSStatus; -- To view, visit https://gerrit.ovirt.org/39763 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I9424a1afefde58b6e687eb4a0325a59ce9573950 Gerrit-PatchSet: 1 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Martin Peřina <mper...@redhat.com> Gerrit-Reviewer: Eli Mesika <emes...@redhat.com> Gerrit-Reviewer: Martin Peřina <mper...@redhat.com> Gerrit-Reviewer: Ori Liel <ol...@redhat.com> Gerrit-Reviewer: Oved Ourfali <oourf...@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
