Mike Kolesnik has posted comments on this change. Change subject: engine: Remove update functionality from AttachNetworkToCluster ......................................................................
Patch Set 2: Code-Review-1 (4 comments) There are some problematic areas in this patch, please see the comments http://gerrit.ovirt.org/#/c/24902/2/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 211 Line 212 Line 213 Line 214 Line 215 You still have to set display & migration exclusively if the network was attached as such.. Line 144: } Line 145: Line 146: private boolean noConflictingNetwork() { Line 147: if (networkExists()) { Line 148: addCanDoActionMessage(VdcBllMessages.NETWORK_ALREADY_ATTACHED_TO_CLUSTER); You can write it more simply as: return failCanDoAction(VdcBllMessages.NETWORK_ALREADY_ATTACHED_TO_CLUSTER); Line 149: return false; Line 150: } Line 151: return true; Line 152: } Line 150: } Line 151: return true; Line 152: } Line 153: Line 154: private boolean networkExists() { Why not just use the get method that accepts NetworkClusterId? Line 155: List<NetworkCluster> networks = getNetworkClusterDAO().getAllForCluster(getVdsGroupId()); Line 156: for (NetworkCluster networkCluster : networks) { Line 157: if (networkCluster.getNetworkId().equals( Line 158: getNetworkCluster().getNetworkId())) { http://gerrit.ovirt.org/#/c/24902/2/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 25: import org.ovirt.engine.core.utils.transaction.TransactionMethod; Line 26: import org.ovirt.engine.core.utils.transaction.TransactionSupport; Line 27: Line 28: @InternalCommandAttribute Line 29: @NonTransactiveCommandAttribute Is there now an UpdateNetworksOnClusterCommand ? The purpose of this command is to support labels feature when multiple networks are being attached/updated on the same cluster. If it will not do the update anymore, there need to be an UpdateNetworksOnClusterCommand that handles this. Line 30: public class AttachNetworksToClusterCommand<T extends ClusterNetworksParameters> extends VdsGroupCommandBase<T> { Line 31: Line 32: public AttachNetworksToClusterCommand(T parameters) { Line 33: super(parameters); -- To view, visit http://gerrit.ovirt.org/24902 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I7aae709dfa0f55ab78d9c1f3caddf75c9d27deb9 Gerrit-PatchSet: 2 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Lior Vernia <lver...@redhat.com> Gerrit-Reviewer: Mike Kolesnik <mkole...@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
