Omer Frenkel has posted comments on this change.
Change subject: engine: watchdog - DB and logic changes
......................................................................
Patch Set 14: I would prefer that you didn't submit this
(8 inline comments)
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/RemoveWatchdogAction.java
Line 5: import org.ovirt.engine.core.common.utils.VmDeviceType;
Line 6: import org.ovirt.engine.core.compat.Guid;
Line 7:
Line 8: @SuppressWarnings("serial")
Line 9: public class RemoveWatchdogAction<T extends VmOperationParameterBase>
extends VmOperationCommandBase<T> {
this will never work, as this is not a command..
if it should be a command, its missing also canDoAction and audit log handling
Line 10:
Line 11: public RemoveWatchdogAction(Guid commandId) {
Line 12: super(commandId);
Line 13: }
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/UpdateTemplateWatchdogCommand.java
Line 13: import org.ovirt.engine.core.common.utils.VmDeviceType;
Line 14: import org.ovirt.engine.core.compat.Guid;
Line 15: import org.ovirt.engine.core.dao.VmDeviceDAO;
Line 16:
Line 17: public class UpdateTemplateWatchdogCommand<T extends
UpdateWatchdogParameters> extends CommandBase<T> {
command is missing canDoAction and audit logging
Line 18:
Line 19: public UpdateTemplateWatchdogCommand(T parameters) {
Line 20: super(parameters);
Line 21: }
Line 16:
Line 17: public class UpdateTemplateWatchdogCommand<T extends
UpdateWatchdogParameters> extends CommandBase<T> {
Line 18:
Line 19: public UpdateTemplateWatchdogCommand(T parameters) {
Line 20: super(parameters);
you should set vm id (actually template id here..) to the command for audit
logging, which also missing
Line 21: }
Line 22:
Line 23: public List<PermissionSubject> getPermissionCheckSubjects() {
Line 24: List<PermissionSubject> permissionList = new
ArrayList<PermissionSubject>();
Line 19: public UpdateTemplateWatchdogCommand(T parameters) {
Line 20: super(parameters);
Line 21: }
Line 22:
Line 23: public List<PermissionSubject> getPermissionCheckSubjects() {
please mark with @Override (also other overriding methods...)
Line 24: List<PermissionSubject> permissionList = new
ArrayList<PermissionSubject>();
Line 25: permissionList.add(new
PermissionSubject(getParameters().getVmId(),
Line 26: VdcObjectType.VmTemplate,
Line 27: getActionType().getActionGroup()));
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/UpdateWatchdogCommand.java
Line 10: import org.ovirt.engine.core.common.utils.VmDeviceType;
Line 11: import org.ovirt.engine.core.compat.Guid;
Line 12: import org.ovirt.engine.core.dal.VdcBllMessages;
Line 13:
Line 14: public class UpdateWatchdogCommand<T extends UpdateWatchdogParameters>
extends VmCommand<T> {
command is missing audit logging
Line 15:
Line 16: public UpdateWatchdogCommand(T parameters) {
Line 17: super(parameters);
Line 18: }
Line 39: VmDevice watchdogDevice = watchdogs.get(0); //there
must be only one
Line 40: watchdogDevice.setSpecParams(getSpecParams());
Line 41: getVmDeviceDao().update(watchdogDevice);
Line 42: }
Line 43: }
if im not wrong, the above code is duplicated with the other update command,
and easily could be shared...
Line 44: setSucceeded(true);
Line 45: }
Line 46:
Line 47: protected HashMap<String, Object> getSpecParams() {
Line 51: return specParams;
Line 52: }
Line 53:
Line 54: protected boolean canDoAction() {
Line 55: if(getVmDAO().get(getParameters().getVmId()) == null) {
if you would set vmId to the command, you could use just getVm() == null
it will also be used when audit logging is added
Line 56: return
failCanDoAction(VdcBllMessages.ACTION_TYPE_FAILED_VM_NOT_FOUND);
Line 57: }
Line 58: return super.canDoAction();
Line 59: }
....................................................
File
backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/action/VdcActionType.java
Line 241: RemoveExternalEvent(1501, ActionGroup.INJECT_EXTERNAL_EVENTS,
QuotaDependency.NONE),
Line 242:
Line 243: UpdateWatchdog(1600, ActionGroup.CREATE_VM, QuotaDependency.NONE),
Line 244: RemoveWatchdog(1601, ActionGroup.CREATE_VM, QuotaDependency.NONE),
Line 245: UpdateTemplateWatchdog(1602, ActionGroup.CREATE_TEMPLATE,
QuotaDependency.NONE),
no remove watchdog for template?
Line 246: ;
Line 247:
Line 248: private int intValue;
Line 249: private ActionGroup actionGroup;
--
To view, visit http://gerrit.ovirt.org/13057
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I813a6f97e23008d15446285998a4e9b50b456040
Gerrit-PatchSet: 14
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Laszlo Hornyak <lhorn...@redhat.com>
Gerrit-Reviewer: Doron Fediuck <dfedi...@redhat.com>
Gerrit-Reviewer: Eli Mesika <emes...@redhat.com>
Gerrit-Reviewer: Gilad Chaplik <gchap...@redhat.com>
Gerrit-Reviewer: Laszlo Hornyak <lhorn...@redhat.com>
Gerrit-Reviewer: Omer Frenkel <ofren...@redhat.com>
_______________________________________________
Engine-patches mailing list
Engine-patches@ovirt.org
http://lists.ovirt.org/mailman/listinfo/engine-patches
