Dudi Maroshi has posted comments on this change. Change subject: engien: Add KSM policy to NUMA hosts ......................................................................
Patch Set 6: (5 comments) https://gerrit.ovirt.org/#/c/39864/6/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/UpdateVdsGroupCommand.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/UpdateVdsGroupCommand.java: Line 134: Line 135: if (getParameters().isForceResetEmulatedMachine()) { Line 136: getParameters().getVdsGroup().setDetectEmulatedMachine(true); Line 137: } Line 138: // if ksm-enabled updated or ksm-merge-across-nodes updated > this section must be called if and when the command ended successfully. see endSuccesfully() is called from runInTransaction() method. Which is not the case for this method. We find no compelling reason to change the method's transaction policy for KSM policy updates. Line 139: if ((getVdsGroup().isKsmMergeAcrossNumaNodes() != getPrevVdsGroup().isKsmMergeAcrossNumaNodes()) || Line 140: (getVdsGroup().isEnableKsm() != getPrevVdsGroup().isEnableKsm())) { Line 141: MomPolicyUpdateParams eventParams = new MomPolicyUpdateParams(getVdsGroup(), allForVdsGroup); Line 142: https://gerrit.ovirt.org/#/c/39864/6/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/VdsEventListener.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/VdsEventListener.java: Line 532: this.allHostsInCluster = allHostsInCluster; Line 533: } Line 534: } Line 535: Line 536: public void onMomPolicyChange(@Observes @MomPolicyUpdate final MomPolicyUpdateParams policyParams) { > TODO this event handling must be called in a separate thread. I need to mak Added TODO comment to the code Line 537: if (policyParams == null || policyParams.cluster == null || policyParams.allHostsInCluster == null) Line 538: return; Line 539: // collect all Active hosts into a callable list Line 540: List<Callable<VDSReturnValue>> callables = new LinkedList<>(); Line 554: } Line 555: Line 556: private VDSReturnValue runUpdateMomPolicy(final VDSGroup cluster, final VDS vds) { Line 557: VDSReturnValue returnValue = new VDSReturnValue(); Line 558: if (cluster.getCompatibilityVersion().compareTo(Version.v3_4) >= 0) { > I'd move this check up to the parent method to spare all iteration and thre Done Line 559: try { Line 560: returnValue = Backend.getInstance().getResourceManager() Line 561: .RunVdsCommand(VDSCommandType.SetMOMPolicyParameters, Line 562: new MomPolicyVDSParameters(vds, https://gerrit.ovirt.org/#/c/39864/6/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/VdsGroupOperationCommandBase.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/VdsGroupOperationCommandBase.java: Line 52: * Get the cluster object as it is in database before update Line 53: * Line 54: * @return Current cluster object before database update, or null if not existing Line 55: */ Line 56: public VDSGroup getPrevVdsGroup() { > shouldn't you justcompare getVdsGroup() vs getParameters().getVdsGropup? public VDSGroup getVdsGroup() { return getParameters().getVdsGroup(); } and getPrevVdsGroup() requires DB query public VDSGroup getPrevVdsGroup() { if (_vdsGroup == null) { _vdsGroup = getVdsGroupDAO().get(getParameters().getVdsGroupId()); } return _vdsGroup; } Line 57: return super.getVdsGroup(); Line 58: } Line 59: Line 60: @Override https://gerrit.ovirt.org/#/c/39864/6/backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/common/qualifiers/MomPolicyUpdate.java File backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/common/qualifiers/MomPolicyUpdate.java: Line 1: package org.ovirt.engine.core.common.qualifiers; Line 2: > missing some declerations on the qualifier Done Line 3: import javax.inject.Qualifier; Line 4: Line 5: @Qualifier Line 6: public @interface MomPolicyUpdate { -- To view, visit https://gerrit.ovirt.org/39864 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I9efa046547fb3732f0fb059092e5f444d72d589c Gerrit-PatchSet: 6 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Dudi Maroshi <d...@redhat.com> Gerrit-Reviewer: Dudi Maroshi <d...@redhat.com> Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Roy Golan <rgo...@redhat.com> Gerrit-Reviewer: automat...@ovirt.org Gerrit-HasComments: Yes _______________________________________________ Engine-patches mailing list Engine-patches@ovirt.org http://lists.ovirt.org/mailman/listinfo/engine-patches
