Arik Hadas has posted comments on this change. Change subject: core: CommandBase - obtain child commands on endAction() ......................................................................
Patch Set 1: (2 comments) https://gerrit.ovirt.org/#/c/41690/1/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 568: } Line 569: Line 570: public void obtainChildCommands() { Line 571: if (getCallBack() != null) { Line 572: List<Guid> childCommands = CommandCoordinatorUtil.getChildCommandIds(getCommandId()); s/childCommands/childCommandIds Line 573: CommandBase<?> commandBase = CommandCoordinatorUtil.retrieveCommand(getCommandId()); Line 574: List<VdcActionParametersBase> parameters = new LinkedList<>(); Line 575: for (Guid id : childCommands) { Line 576: CommandBase<?> command = CommandCoordinatorUtil.retrieveCommand(id); Line 579: parameters.add(command.getParameters()); Line 580: } Line 581: } Line 582: Line 583: commandBase.getParameters().setImagesParameters(parameters); > I do see your point, but can't we say the same about locks, jobs, endAction to be honest, I don't understand this code - the parameters are added to 'commandBase' which is not used anywhere later on.. I thought that maybe it works because CommandCoordinatorUtil.retrieveCommand returns a cached command but it seems that it is not the case. don't you need to update the parameters of the instance we're in instead? putting that aside, I also think that it is not the right place for this kind of code since: 1. This class should imho should remain clean of not-general bll entity names such as vm. image, network interface etc 2. Let's say that we'll want to cleanup the parameters classes so the parameters class of a command such as MigrateVmCommand, which has nothing to do with images, won't have methods such as setImageParameters. Once we do it, it will be more obvious that this case is too specific 3. Looking at the previous async-task, it used to be in the async-tasks area (which was refactored to CommandAsyncTask). I still think it is a more related to the tasks/commands-jobs handling than to command instance I do agree that it needs to be handled by some kind of infra so maybe a storage-based callback that does it for the command is the right place for it. Line 584: } Line 585: } Line 586: Line 587: protected boolean isEndSuccessfully() { -- To view, visit https://gerrit.ovirt.org/41690 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ia563dc1b97e0df852e242420eca4f987a39fc2b9 Gerrit-PatchSet: 1 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Liron Aravot <lara...@redhat.com> Gerrit-Reviewer: Arik Hadas <aha...@redhat.com> Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Liron Aravot <lara...@redhat.com> Gerrit-Reviewer: Moti Asayag <masa...@redhat.com> Gerrit-Reviewer: Oved Ourfali <oourf...@redhat.com> Gerrit-Reviewer: Ravi Nori <rn...@redhat.com> Gerrit-Reviewer: automat...@ovirt.org Gerrit-HasComments: Yes _______________________________________________ Engine-patches mailing list Engine-patches@ovirt.org http://lists.ovirt.org/mailman/listinfo/engine-patches
