Arik Hadas has posted comments on this change. Change subject: engine: Added init method to command base. ......................................................................
Patch Set 2: (1 comment) http://gerrit.ovirt.org/#/c/37110/2/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/CommandBase.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/CommandBase.java: Line 1057: addCanDoActionMessage(VdcBllMessages.FAILED_TO_INITIALIZE); Line 1058: return false; Line 1059: } Line 1060: return true; Line 1061: } > I sent the original patch with "init" so at some point I thought it's good, No, validity of parameters is not common in our initialization code since can-do-action checks should verify them. Sure, in general you should not ignore exceptions. But in our case exceptions in the initialization should be ignored because: 1. can do action should catch them and return appropriate error the the user. for example we have such cases in the system: constcutor(Parameters p) { super.. if (getVm() != null) { setStorageDomainId(getVm().getStorageId(); } } I don't want you to return 'false' for this kind of code if the VM is null (yeah, the initialization failed, we could not initialize the storage domain, but I want you to continue since the canDoAction will return appropriate message to the user. 2. No complicated logic with can fail should be executed before the can do action phase. just settings of entity from the parameters. 3. If canDoAction phase passes, the command should be able execute. If canDoAction succeeded, but the command could not execute, you should add/fix your checks. I'm not arguing about the general approach for proper validations, all I'm saying is that in our infrastructure for commands, no need for another phase for initialization that can fail. that's why I asked Amit to provide examples for usages, maybe they plan to add something we don't have in the system yet.. Line 1062: Line 1063: /** Line 1064: * This method should be inherited by subclasses that would like to perform initialization after permission check Line 1065: * -- To view, visit http://gerrit.ovirt.org/37110 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I0c71548a8fab5538ee97c279f12a821999635950 Gerrit-PatchSet: 2 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Yair Zaslavsky <yzasl...@redhat.com> Gerrit-Reviewer: Amit Aviram <aavi...@redhat.com> Gerrit-Reviewer: Arik Hadas <aha...@redhat.com> Gerrit-Reviewer: Oved Ourfali <oourf...@redhat.com> Gerrit-Reviewer: Ravi Nori <rn...@redhat.com> Gerrit-Reviewer: Yair Zaslavsky <yzasl...@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
