Liron Aravot has posted comments on this change. Change subject: core: CommandBase - obtain child commands on endAction() ......................................................................
Patch Set 1: (1 comment) 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 579: parameters.add(command.getParameters()); Line 580: } Line 581: } Line 582: Line 583: commandBase.getParameters().setImagesParameters(parameters); > to be honest, I don't understand this code - the parameters are added to 'c Thanks for the review Arik, *The code is written like that because it was moved to here, it's working because on the relevant scenarios it's the same instance (commandBase and the object we are running on) but obviously it should be changed to 'this' instead. *The fact that the member is named "ImageParameters" is incorrect generally, it should be named "AsyncOperationParameters" even with the async tasks infra (async tasks can be used for anything, not just images/storage) and i'm in favour of changing that. I argue that this code isn't related solely to images, under the CoCo infrastructure you should execute a command and poll it (it can be a vm migration, host configuration, etc), this code correlates with that to align with the other coco relevant parts the exist here. while the end methods resides in the commandbase (those methods are relevant only to commands that executes async commands, whether they are storage or not) this addition should reside here as well imo as it's generic and shouldn't be copied over and over. As stated before, i do agree with you and Moti that ImageParameters can and should be renamed on a different patch and if we'll agree on leaving it here i'll change 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
