Michael Kublin has posted comments on this change. Change subject: engine-core: add plug/unplug VM nic command ......................................................................
Patch Set 1: (7 inline comments) .................................................... File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/HotPlugUnplugVmNicCommand.java Line 14: */ Mark as non transactive, I don't think that u need transaction here Line 17: VmDevice vmDevice; these is ugly, getVmDeviceDAO() defined at some parent class, use it Line 25: @Override First of all, why not change a status to plug/unplug when vm is down. Can be done for any version of cluster/vm Line 53: why u did not update status of device as plugged/unplugged? Line 56: the method with the same name exist in CommandBase, any relations? Line 65: Consider add AuditLogMessage on command success or failure .................................................... File backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/PlugAction.java Line 4: Consider to use boolean , especially when u have enum only with two options -- To view, visit http://gerrit.ovirt.org/3204 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I17897c18eccae41954e01b264fa74427e0bce1a2 Gerrit-PatchSet: 1 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Roy Golan <[email protected]> Gerrit-Reviewer: Michael Kublin <[email protected]> _______________________________________________ Engine-patches mailing list [email protected] http://lists.ovirt.org/mailman/listinfo/engine-patches
