Yevgeny Zaspitsky has posted comments on this change. Change subject: engine: Refactor AttachNetworksToClusterCommand ......................................................................
Patch Set 9: (23 comments) http://gerrit.ovirt.org/#/c/33684/9/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/network/cluster/AttachNetworkToClusterCommand.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/network/cluster/AttachNetworkToClusterCommand.java: Line 22: import org.ovirt.engine.core.dao.VmDAO; Line 23: import org.ovirt.engine.core.dao.network.NetworkClusterDao; Line 24: Line 25: @InternalCommandAttribute Line 26: public class AttachNetworkToClusterCommand<T extends AttachNetworkToVdsGroupParameter> extends > Please rename the class. You have AttachNetworkToVdsGroup and now you call Done Line 27: NetworkClusterCommandBase<T> { Line 28: Line 29: AttachNetworkToClusterCommand(T parameters) { Line 30: super(parameters, null); Line 42: setSucceeded(true); Line 43: } Line 44: Line 45: @Override Line 46: protected boolean canDoAction() { > Moving the canDoAction to here it problematic. Since the ui executes- runMu Now UI runs single AttachNetworksToCluster action Line 47: return networkNotAttachedToCluster() Line 48: && vdsGroupExists() Line 49: && changesAreClusterCompatible() Line 50: && logicalNetworkExists() http://gerrit.ovirt.org/#/c/33684/9/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/network/cluster/AttachNetworkToVdsGroupCommand.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/network/cluster/AttachNetworkToVdsGroupCommand.java: Line 8: import org.ovirt.engine.core.common.action.VdcActionParametersBase; Line 9: import org.ovirt.engine.core.common.action.VdcActionType; Line 10: import org.ovirt.engine.core.common.action.VdcReturnValueBase; Line 11: import org.ovirt.engine.core.compat.TransactionScopeOption; Line 12: > Why should this command be transactive? The only transactive part is callin Done Line 13: public class AttachNetworkToVdsGroupCommand<T extends AttachNetworkToVdsGroupParameter> extends VdsGroupCommandBase<T> { Line 14: Line 15: public AttachNetworkToVdsGroupCommand(T parameters, CommandContext cmdContext) { Line 16: super(parameters, cmdContext); Line 28: parameters.setTransactionScopeOption(TransactionScopeOption.Required) > Should be changed to- RequiredNew. I do not see a good enough reason for making this RequiredNew. In case of this code will be run from within a transaction we do not want another transaction to be opened. Line 25: Line 26: final T parameters = getParameters(); Line 27: Line 28: parameters.setTransactionScopeOption(TransactionScopeOption.Required); Line 29: parameters.setCompensationEnabled(false); > Why do you need this? Done Line 30: Line 31: final VdcReturnValueBase returnValue = Line 32: getBackend().runInternalAction(VdcActionType.AttachNetworkToVdsGroup, parameters); Line 33: Line 28: parameters.setTransactionScopeOption(TransactionScopeOption.Required); Line 29: parameters.setCompensationEnabled(false); Line 30: Line 31: final VdcReturnValueBase returnValue = Line 32: getBackend().runInternalAction(VdcActionType.AttachNetworkToVdsGroup, parameters); > The CommandBase has runInternalAction method. It should be called in order Done Line 33: Line 34: final boolean attachSucceed = returnValue.getSucceeded(); Line 35: setSucceeded(attachSucceed); Line 36: Line 41: Line 42: private void attachLabeledNetworks() { Line 43: final T parameters = getParameters(); Line 44: Line 45: parameters.setTransactionScopeOption(TransactionScopeOption.Suppress); > Can be removed. Done Line 46: parameters.setCompensationEnabled(false); Line 47: Line 48: // AttachLabeledNetworks should be run asynchronously Line 49: runInternalMultipleActions(VdcActionType.PropagateLabeledNetworksToClusterHosts, Line 42: private void attachLabeledNetworks() { Line 43: final T parameters = getParameters(); Line 44: Line 45: parameters.setTransactionScopeOption(TransactionScopeOption.Suppress); Line 46: parameters.setCompensationEnabled(false); > Again- why do you need this? Done Line 47: Line 48: // AttachLabeledNetworks should be run asynchronously Line 49: runInternalMultipleActions(VdcActionType.PropagateLabeledNetworksToClusterHosts, Line 50: createParametersArrayList(parameters)); Line 46: parameters.setCompensationEnabled(false); Line 47: Line 48: // AttachLabeledNetworks should be run asynchronously Line 49: runInternalMultipleActions(VdcActionType.PropagateLabeledNetworksToClusterHosts, Line 50: createParametersArrayList(parameters)); > Why do you run this command as multiple actions? that's the way to run a single command asynchronously Line 51: } Line 52: Line 53: private ArrayList<VdcActionParametersBase> createParametersArrayList(final T parameters) { Line 54: final ArrayList<VdcActionParametersBase> resultList = new ArrayList<>(); Line 49: runInternalMultipleActions(VdcActionType.PropagateLabeledNetworksToClusterHosts, Line 50: createParametersArrayList(parameters)); Line 51: } Line 52: Line 53: private ArrayList<VdcActionParametersBase> createParametersArrayList(final T parameters) { > Same- as I understand there is no need to run the command as multiple actio that's the way to run a single command asynchronously Line 54: final ArrayList<VdcActionParametersBase> resultList = new ArrayList<>(); Line 55: resultList.add(parameters); Line 56: Line 57: return resultList; http://gerrit.ovirt.org/#/c/33684/9/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/network/cluster/AttachNetworksToClusterCommand.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/network/cluster/AttachNetworksToClusterCommand.java: Line 36: protected void executeCommand() { Line 37: Line 38: final boolean attachNetworksToClusterResult = attachNetworksToCluster(); Line 39: Line 40: setSucceeded(attachNetworksToClusterResult); > Shouldn't you return in case attachNetworksToClusterResult is false? (altho Done Line 41: Line 42: // PropagateLabeledNetworksToClusterHosts should be run asynchronously Line 43: runInternalMultipleActions(VdcActionType.PropagateLabeledNetworksToClusterHosts, Line 44: wrapParametersWithList(getParameters())); Line 39: Line 40: setSucceeded(attachNetworksToClusterResult); Line 41: Line 42: // PropagateLabeledNetworksToClusterHosts should be run asynchronously Line 43: runInternalMultipleActions(VdcActionType.PropagateLabeledNetworksToClusterHosts, > If you have just one parameter, why do you run it a multiple action? That's the way to run a command asynchronously. This what the comment above is about. Moved to CommandBase Line 44: wrapParametersWithList(getParameters())); Line 45: } Line 46: Line 47: private ArrayList<VdcActionParametersBase> wrapParametersWithList(T parameters) { http://gerrit.ovirt.org/#/c/33684/9/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/network/cluster/NetworkClusterCommandBase.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/network/cluster/NetworkClusterCommandBase.java: Line 16: } Line 17: Line 18: protected NetworkClusterCommandBase(T parameters) { Line 19: super(parameters); Line 20: setVdsGroupId(parameters.getVdsGroupId()); > You've added this line to VdsGroupCommandBase, so you don't need it here an Done Line 21: } Line 22: Line 23: protected NetworkCluster getNetworkCluster() { Line 24: return getParameters().getNetworkCluster(); Line 40: && validate(validator.externalNetworkNotDisplay(getNetworkName())) Line 41: && validate(validator.externalNetworkNotRequired(getNetworkName())); Line 42: } Line 43: Line 44: protected Version getClusterVersion() { > You should remove the override of this method from UpdateNetworkOnClusterCo the method was moved as a private one to UpdateNetworkOnClusterCommand Line 45: return getVdsGroup().getcompatibility_version(); Line 46: } Line 47: Line 48: protected boolean validateAttachment() { http://gerrit.ovirt.org/#/c/33684/9/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/network/cluster/SetManagementNetworkFirstParameterComparator.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/network/cluster/SetManagementNetworkFirstParameterComparator.java: Line 1: package org.ovirt.engine.core.bll.network.cluster; > Please move to org.ovirt.engine.core.common.businessentities.comparators The class was removed Line 2: Line 3: import static org.apache.commons.lang.BooleanUtils.toInteger; Line 4: Line 5: import java.util.Comparator; Line 14: public int compare(T param1, T param2) { Line 15: final boolean management1 = isManagementNetworkAppointmentCommand(param1); Line 16: final boolean management2 = isManagementNetworkAppointmentCommand(param2); Line 17: Line 18: return toInteger(management2) - toInteger(management1); > 1. Shouldn't you return toInteger(management1) - toInteger(management2)? the sort order is increasing, so in order to have management params first they had to have low values. Hence that should 2-1 and not 1-2 Line 19: } Line 20: Line 21: private boolean isManagementNetworkAppointmentCommand(T param) { Line 22: final Network network = param.getNetwork(); Line 17: Line 18: return toInteger(management2) - toInteger(management1); Line 19: } Line 20: Line 21: private boolean isManagementNetworkAppointmentCommand(T param) { > Maybe- isManagementNetworkParameter? Done Line 22: final Network network = param.getNetwork(); Line 23: if (network == null) { Line 24: return false; Line 25: } Line 18: return toInteger(management2) - toInteger(management1); Line 19: } Line 20: Line 21: private boolean isManagementNetworkAppointmentCommand(T param) { Line 22: final Network network = param.getNetwork(); > You don't need this check, you can call directy- param.getNetworkCluster() Done Line 23: if (network == null) { Line 24: return false; Line 25: } Line 26: http://gerrit.ovirt.org/#/c/33684/9/backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/network/cluster/AttachNetworkToClusterCommandTest.java File backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/network/cluster/AttachNetworkToClusterCommandTest.java: Line 65: underTest = new AttachNetworkToClusterCommand<AttachNetworkToVdsGroupParameter>(param); Line 66: } Line 67: Line 68: private void initializeDbFacadeMocks() { Line 69: DbFacadeLocator.setDbFacade(mockDbFacade); > As I wrote and explained in the previous patches- please don't use the loca Done Line 70: when(mockDbFacade.getNetworkDao()).thenReturn(mockNetworkDao); Line 71: when(mockDbFacade.getNetworkClusterDao()).thenReturn(mockNetworkClusterDao); Line 72: when(mockDbFacade.getVdsGroupDao()).thenReturn(mockVdsGroupDAO); Line 73: } http://gerrit.ovirt.org/#/c/33684/9/backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/network/cluster/SetManagementNetworkFirstParameterComparatorTest.java File backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/network/cluster/SetManagementNetworkFirstParameterComparatorTest.java: Line 14: import org.ovirt.engine.core.common.businessentities.network.Network; Line 15: import org.ovirt.engine.core.common.businessentities.network.NetworkCluster; Line 16: Line 17: @RunWith(MockitoJUnitRunner.class) Line 18: public class SetManagementNetworkFirstParameterComparatorTest { > I'm not sure you still need the comparator. Added comments in case you'll s The class is removed Line 19: Line 20: @Mock Line 21: private AttachNetworkToVdsGroupParameter mockAttachNetworkToVdsGroupParameter1; Line 22: @Mock Line 48: testCompareNoNulls(true, false, -1); Line 49: } Line 50: Line 51: @Test Line 52: public void compareEq1() { > maybe- compareEqTrue? Done Line 53: testCompareNoNulls(true, true, 0); Line 54: } Line 55: Line 56: @Test Line 53: testCompareNoNulls(true, true, 0); Line 54: } Line 55: Line 56: @Test Line 57: public void compareEq2() { > maybe- compareEqFalse? Done Line 58: testCompareNoNulls(false, false, 0); Line 59: } Line 60: Line 61: @Test Line 58: testCompareNoNulls(false, false, 0); Line 59: } Line 60: Line 61: @Test Line 62: public void compareWithNulls1() { > 1. Please give more informative name than 1 and 2. Done Line 63: assertEquals(0, underTest.compare(mockAttachNetworkToVdsGroupParameter1, mockAttachNetworkToVdsGroupParameter2)); Line 64: when(mockAttachNetworkToVdsGroupParameter1.getNetwork()).thenReturn(mockNetwork1); Line 65: assertEquals(0, underTest.compare(mockAttachNetworkToVdsGroupParameter1, mockAttachNetworkToVdsGroupParameter2)); Line 66: when(mockNetwork1.getCluster()).thenReturn(mockNetworkCluster1); -- To view, visit http://gerrit.ovirt.org/33684 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I6a0b86d8bb018a6172891cb357a4402cfef135d1 Gerrit-PatchSet: 9 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Yevgeny Zaspitsky <[email protected]> Gerrit-Reviewer: Alona Kaplan <[email protected]> Gerrit-Reviewer: Moti Asayag <[email protected]> Gerrit-Reviewer: Yevgeny Zaspitsky <[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
