Alex Lourie has posted comments on this change. Change subject: packaging: Updated interaction with user on async tasks ......................................................................
Patch Set 3: (2 inline comments) .................................................... Commit Message Line 10: the user, where tasks were found and user was asked whether to continue Line 11: (and if user continues, then restart engine in maintenance mode) Line 12: Line 13: If user chooses not to continue, 'finally' will kick in, and try to Line 14: restore the engine configuration, although it didn't change yet. I disagree. Of course, there are checks in the function itself, that prevent it from crashing if, for instance, it tries to remove files that do not exists, but I don't think that it should be smarter than that and understand *when* to run. The 'enterMaintenance' and 'exitMaintenance' functions are just utility functions, they do exactly what they are called. The decision to 'configure for maintenance' or 'restore from maintenance' must be external. Besides, the using 'try' before actually anything happens, has no added value in our case, because should exception happen on that stage - it will be safely caught in the main loop. As such, the decision of the current code to 'restore from Maintenance' even if the system was not in the maintenance yet is wrong, and this patch attempts to correct this behaviour. Line 15: Line 16: * Also, changed the return value in getRunningTasks and getCompensation Line 17: functions from "return None" to empty string "return ''". This improves Line 18: the printouts to the user, and instead of printing "None" in tasks, .................................................... File packaging/fedora/setup/engine-upgrade.py Line 845: print timestamp + output_messages.INFO_STOPPING_TASKS % MAINTENANCE_TASKS_WAIT_PERIOD_MINUTES Line 846: Line 847: # restart jboss/engine in maintenace mode (i.e different port): Line 848: utils.configureEngineForMaintenance() Line 849: origTimeout = utils.configureTasksTimeout(timeout=0, No. 1. origTimeout is already defined by this point. 2. The problem is not in timeout, but in 'restore from maintenance' - if the use decided not to continue on the first question, the finally will cause restoration to work, even though no reconfiguration was done yet. Line 850: engineConfigBin=engineConfigBinary, Line 851: engineConfigExtended=engineConfigExtended) Line 852: startEngine(service) Line 853: -- To view, visit http://gerrit.ovirt.org/8952 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ia1f1aaf7ac812d69dc81f3fb8dee5a626bc7634a Gerrit-PatchSet: 3 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Alex Lourie <alou...@redhat.com> Gerrit-Reviewer: Alex Lourie <alou...@redhat.com> Gerrit-Reviewer: Alon Bar-Lev <alo...@redhat.com> Gerrit-Reviewer: Juan Hernandez <juan.hernan...@redhat.com> Gerrit-Reviewer: Kiril Nesenko <knese...@redhat.com> Gerrit-Reviewer: Moran Goldboim <mgold...@redhat.com> Gerrit-Reviewer: Ofer Schreiber <oschr...@redhat.com> Gerrit-Reviewer: Ohad Basan <oba...@redhat.com> _______________________________________________ Engine-patches mailing list Engine-patches@ovirt.org http://lists.ovirt.org/mailman/listinfo/engine-patches
