Allon Mureinik has posted comments on this change. Change subject: engine: Commands to add, edit and remove iscsi bonds ......................................................................
Patch Set 7: Code-Review-1 (10 comments) @Sergey - see comments on the code inline. @Cheryn, could you please take a look at the new messages in backend/manager/modules/dal/src/main/resources/bundles/AppErrors.properties and backend/manager/modules/dal/src/main/resources/bundles/AuditLogMessages.properties ? Thanks! http://gerrit.ovirt.org/#/c/22952/7/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/AddIscsiBondCommand.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/AddIscsiBondCommand.java: Line 1: package org.ovirt.engine.core.bll; shouldn't this be in bll.storage? Line 2: Line 3: import java.util.Collections; Line 4: import java.util.List; Line 5: Line 12: import org.ovirt.engine.core.compat.Guid; Line 13: import org.ovirt.engine.core.utils.transaction.TransactionMethod; Line 14: import org.ovirt.engine.core.utils.transaction.TransactionSupport; Line 15: Line 16: @LockIdNameAttribute(isReleaseAtEndOfExecute = false) why false? you don't have an async task here. Line 17: @NonTransactiveCommandAttribute Line 18: public class AddIscsiBondCommand<T extends AddIscsiBondParameters> extends CommandBase<T> { Line 19: Line 20: public AddIscsiBondCommand(T parameters) { Line 24: Line 25: public AddIscsiBondCommand(Guid commandId) { Line 26: super(commandId); Line 27: } Line 28: How about having a canDoAction() to check that this entity doesn't exist yet? Line 29: @Override Line 30: protected void executeCommand() { Line 31: final List<Guid> networkIds = getParameters().getIscsiBond().getNetworkIds(); Line 32: final List<String> connectionsIds = getParameters().getIscsiBond().getStorageConnectionIds(); Line 33: final IscsiBond iscsiBond = getParameters().getIscsiBond(); Line 34: Line 35: iscsiBond.setId(Guid.newGuid()); Line 36: Line 37: TransactionSupport.executeInNewTransaction(new TransactionMethod<Void>() { Why do you a new transaction? (come to think of it - why is the command non transactive? you have no VDSM calls here... Line 38: Line 39: @Override Line 40: public Void runInTransaction() { Line 41: getDbFacade().getIscsiBondDao().save(iscsiBond); Line 40: public Void runInTransaction() { Line 41: getDbFacade().getIscsiBondDao().save(iscsiBond); Line 42: Line 43: for (Guid networkId : networkIds) { Line 44: getDbFacade().getIscsiBondDao().addNetworkToIscsiBond(iscsiBond.getId(), networkId); This should be batched - make the dao extend MassOperationsGenericDaoDbFacade, and you should get this implementation for free. Line 45: } Line 46: Line 47: for (String connectionId : connectionsIds) { Line 48: getDbFacade().getIscsiBondDao().addStorageConnectionToIscsiBond(iscsiBond.getId(), connectionId); Line 44: getDbFacade().getIscsiBondDao().addNetworkToIscsiBond(iscsiBond.getId(), networkId); Line 45: } Line 46: Line 47: for (String connectionId : connectionsIds) { Line 48: getDbFacade().getIscsiBondDao().addStorageConnectionToIscsiBond(iscsiBond.getId(), connectionId); this one too Line 49: } Line 50: Line 51: getCompensationContext().stateChanged(); Line 52: return null; http://gerrit.ovirt.org/#/c/22952/7/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/EditIscsiBondCommand.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/EditIscsiBondCommand.java: Line 1: package org.ovirt.engine.core.bll; shouldn't this be in bll.storage? Line 2: Line 3: import java.util.Collections; Line 4: import java.util.HashSet; Line 5: import java.util.List; Line 40: Line 41: @Override Line 42: protected void executeCommand() { Line 43: Line 44: TransactionSupport.executeInNewTransaction(new TransactionMethod<Void>() { There is no VDSM action here, only database - just make the command transactive, and save yourself all this hassle. Line 45: Line 46: @Override Line 47: public Void runInTransaction() { Line 48: if (!StringUtils.equals(getNewIscsiBond().getName(), getExistingIscsiBond().getName()) || http://gerrit.ovirt.org/#/c/22952/7/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/RemoveIscsiBondCommand.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/RemoveIscsiBondCommand.java: Line 1: package org.ovirt.engine.core.bll; shouldn't this be in bll.storage? Line 2: Line 3: import java.util.Collections; Line 4: import java.util.List; Line 5: Line 30: } Line 31: Line 32: @Override Line 33: protected void executeCommand() { Line 34: TransactionSupport.executeInNewTransaction(new TransactionMethod<Object>() { Just make the command transactive Line 35: @Override Line 36: public Void runInTransaction() { Line 37: getDbFacade().getIscsiBondDao().remove(getParameters().getIscsiBondId()); Line 38: return null; -- To view, visit http://gerrit.ovirt.org/22952 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I06890ea40d22b86d9421cb1b0c68ea28bc430b0a Gerrit-PatchSet: 7 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Sergey Gotliv <sgot...@redhat.com> Gerrit-Reviewer: Allon Mureinik <amure...@redhat.com> Gerrit-Reviewer: Daniel Erez <de...@redhat.com> Gerrit-Reviewer: Maor Lipchuk <mlipc...@redhat.com> Gerrit-Reviewer: Sergey Gotliv <sgot...@redhat.com> 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
