Hi Geoffrey, On 07/06/2012 23:12, Geoffrey Hart wrote: > Karen, Darren: > > I know that both of you are now happy with the code change (assuming > Darren likes the latest webrev listed below). But I do have to ask a > question that might result in a slight change to this. Darren requested > that I make the logging INFO so it will appear on the console, but I'm > having second thoughts about this. Currently the console shows progress > with text like: > > Install Phase 20958/86138 > > This allows the user in a clear fashion to see it progressing so we know > it is not hung and we can see how fast it is progressing. By allowing my > new logging to go to the console we see the output become more cluttered: > > Install Phase 23252/86138 14:59:12 Install > Phase . > .. 26% complete. > Install Phase 29649/86138 15:00:12 Install > Phase . > .. 34% complete. > Install Phase 33658/86138 15:01:12 Install > Phase . > .. 39% complete. > Install Phase 34629/86138 15:02:12 Install > Phase . > .. 40% complete. > Install Phase 35574/86138 15:03:12 Install > Phase . > .. 41% complete. >
This is where you are seeing the mixed content of the show_stdout=True and the new logging you're doing. I think that if there is no tty associated with the output, then it shouldn't be generating the numbers x/X. Certainly we should handle this case and only have one of them depending on the circumstance. > > > Is there a good reason to have this extra logging to the console? Or > does it make more sense to just have it go to the log file and keep the > current look to the console? If you are seeing the x/X type output on an AI console, where you're not logged in, then I think that is strange, since I wouldn't expect it. You need to ensure only one of these is output, depending on whether there is a tty - since without a tty, the x/X type output really doesn't work correctly. Thanks, Darren. > > Your thoughts? > > Latest webrev: > https://cr.opensolaris.org/action/browse/caiman/ghart/IPS_Install_Progress_V3/ > > - Geoffrey > > On 06/07/12 16:06, Karen Tung wrote: >> The updated info on the bug looks OK. >> I have no further comments. >> >> --Karen >> >> On 06/07/12 12:12, Geoffrey Hart wrote: >>> Sorry, looks like I didn't hit save on the bug. Updated now. >>> >>> - Geoffrey >>> >>> On 06/07/12 12:47, Karen Tung wrote: >>>> Hi Geoffrey, >>>> >>>> Sorry I overlooked your email too. I just looked at the updated >>>> webrev. >>>> The code changes look OK to me now. >>>> >>>> However, the bug is still not updated. Can you update the bug? >>>> >>>> Thanks, >>>> >>>> --Karen >>>> >>>> On 06/05/12 08:44, Geoffrey Hart wrote: >>>>> Updated webrev with suggested changes: >>>>> >>>>> https://cr.opensolaris.org/action/browse/caiman/ghart/IPS_Install_Progress_V2/webrev/ >>>>> >>>>> >>>>> >>>>> On 06/05/12 02:01, Darren Kenny wrote: >>>>>> >>>>>> On 01/06/2012 22:56, Geoffrey Hart wrote: >>>>>>> Yes we have everything downloaded already. I didn;t add anything for >>>>>>> progress on the download phase since it already shows the >>>>>>> progress for >>>>>>> download on the console. The motivation for this fix is the 10 or so >>>>>>> minutes it takes for install where we do no logging to the >>>>>>> console or >>>>>>> log file. >>>>>>> >>>>>> OK, thanks, just wanted to be sure... :) >>>>>> >>>>>>> As for as displaying to the console, very good point. I will correct >>>>>>> that to show status to the console. >>>>>> Thanks. >>>>>> >>>>>>> So with this in mind, what are your thoughts on logging every >>>>>>> minute vs >>>>>>> logging after every 5 or 10 %? >>>>>> Every minute is fine - so even if there is some long install task >>>>>> (which I >>>>>> hope there isn't anything longer than 1 minute TBH) it would still >>>>>> show >>>>>> some output so the user doesn't think things have stalled... >>>>>> >>>>>> Thanks, >>>>>> >>>>>> Darren. >>>>>> >>>>>>> Thanks, >>>>>>> Geoffrey >>>>>>> >>>>>>> >>>>>>> On 06/01/12 10:25, Darren Kenny wrote: >>>>>>>> On Fri Jun 1 17:11:44 2012, Geoffrey Hart wrote: >>>>>>>>> Thanks Darren. >>>>>>>>> >>>>>>>>> I will run this through pep8 as I make the other corrections >>>>>>>>> you mentioned. >>>>>>>>> >>>>>>>>> I selected to log every minute rather than every 5% so there >>>>>>>>> would be >>>>>>>>> regular log entries. With network problems, it could be a long >>>>>>>>> delay >>>>>>>>> between entries if I logged every 5% of the way. Your thoughts? >>>>>>>>> >>>>>>>> I can see where you're coming from, but at this point - install >>>>>>>> - we've >>>>>>>> already downloaded everything, haven't we? >>>>>>>> >>>>>>>> Or are you adding the progress at the download phase too - I'm not >>>>>>>> totally >>>>>>>> sure of of the phases based on the names in the webrev... >>>>>>>> >>>>>>>> BTW, does any of this appear on the console during an AI install? >>>>>>>> >>>>>>>> Ideally we want it to, so people will see progress without >>>>>>>> logging in, >>>>>>>> and I'm thinking that it would need to be at the INFO >>>>>>>> level rather than DEBUG level for that to be the case. >>>>>>>> >>>>>>>> Thanks, >>>>>>>> >>>>>>>> Darren. >>>>>>>> >>>>>>>>> - Geoffrey >>>>>>>>> >>>>>>>>> On 06/01/12 04:39, Darren Kenny wrote: >>>>>>>>>> Hi Geoffrey, >>>>>>>>>> >>>>>>>>>> Thanks for looking at this... >>>>>>>>>> >>>>>>>>>> In general I'm happy with the approach, but have some minor >>>>>>>>>> comments in >>>>>>>>>> addition to Karen's: >>>>>>>>>> >>>>>>>>>> - You could probably omit the need for the percent_complete >>>>>>>>>> variable >>>>>>>>>> totally, if you changed the lines to look like: >>>>>>>>>> >>>>>>>>>> if time_diff.seconds>= 60: >>>>>>>>>> self._logger_output("%s%% complete." % >>>>>>>>>> (100 * self.act_cur_nactions) / >>>>>>>>>> self.act_goal_nactions) >>>>>>>>>> >>>>>>>>>> - Another approach, rather than using time, would have been to >>>>>>>>>> just >>>>>>>>>> remember the last percentage calculated, and just report >>>>>>>>>> every 5%, >>>>>>>>>> or similar. >>>>>>>>>> >>>>>>>>>> Either works for me - and in someways 60 seconds is >>>>>>>>>> better, just >>>>>>>>>> wondering if you considered this approach and ruled it out? >>>>>>>>>> >>>>>>>>>> - The indentation around the percentage calculation is >>>>>>>>>> confusing, and >>>>>>>>>> possible not PEP8 clean - have you run PEP8 against the >>>>>>>>>> code? >>>>>>>>>> >>>>>>>>>> An example of this is: >>>>>>>>>> >>>>>>>>>> if time_diff.seconds>= 60: >>>>>>>>>> percent_complete = (100 * >>>>>>>>>> self.act_cur_nactions)/self.act_goal_nactions >>>>>>>>>> self._logger_output("%s%% complete." % >>>>>>>>>> percent_complete) >>>>>>>>>> >>>>>>>>>> this would be more readable, if calculation was indented >>>>>>>>>> differently, >>>>>>>>>> (and also PEP8 cleaner), >>>>>>>>>> >>>>>>>>>> if time_diff.seconds>= 60: >>>>>>>>>> percent_complete = (100 * >>>>>>>>>> self.act_cur_nactions) / self.act_goal_nactions >>>>>>>>>> self._logger_output("%s%% complete." % >>>>>>>>>> percent_complete) >>>>>>>>>> >>>>>>>>>> or even: >>>>>>>>>> >>>>>>>>>> if time_diff.seconds>= 60: >>>>>>>>>> percent_complete = \ >>>>>>>>>> (100 * self.act_cur_nactions) / >>>>>>>>>> self.act_goal_nactions >>>>>>>>>> self._logger_output("%s%% complete." % >>>>>>>>>> percent_complete) >>>>>>>>>> >>>>>>>>>> My preference would be the latter... >>>>>>>>>> >>>>>>>>>> Thanks, >>>>>>>>>> >>>>>>>>>> Darren. >>>>>>>>>> >>>>>>>>>> >>>>>>>>>>> Afternoon all! >>>>>>>>>>> >>>>>>>>>>> Could I please have a code review for: >>>>>>>>>>> >>>>>>>>>>> BUG: 7171973 IPS transfer code should show more logging >>>>>>>>>>> during install >>>>>>>>>>> of packages >>>>>>>>>>> >>>>>>>>>>> CODE REVIEW: >>>>>>>>>>> https://cr.opensolaris.org/action/browse/caiman/ghart/IPS_Install_Progress/ >>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> DESCRIPTION: Adding progress logging during the install of >>>>>>>>>>> IPS packages >>>>>>>>>>> so during the 10+ minutes of install time there are now log >>>>>>>>>>> entries to >>>>>>>>>>> show the install is proceeding and not hung. >>>>>>>>>>> >>>>>>>>>>> OLD LOG ENTRY: >>>>>>>>>>> 2012-05-29 16:19:30,799 >>>>>>>>>>> InstallationLogger.generated-transfer-1653-1 >>>>>>>>>>> DEBUG Install Phase ... Started. >>>>>>>>>>> 2012-05-29 16:31:40,035 >>>>>>>>>>> InstallationLogger.generated-transfer-1653-1 >>>>>>>>>>> DEBUG Install Phase ... Done. >>>>>>>>>>> >>>>>>>>>>> NEW LOG ENTRY: >>>>>>>>>>> 2012-05-29 16:19:30,799 >>>>>>>>>>> InstallationLogger.generated-transfer-1653-1 >>>>>>>>>>> DEBUG Install Phase ... Started. >>>>>>>>>>> 2012-05-29 16:20:30,803 >>>>>>>>>>> InstallationLogger.generated-transfer-1653-1 >>>>>>>>>>> DEBUG 24% complete. >>>>>>>>>>> 2012-05-29 16:21:30,804 >>>>>>>>>>> InstallationLogger.generated-transfer-1653-1 >>>>>>>>>>> DEBUG 32% complete. >>>>>>>>>>> 2012-05-29 16:22:32,195 >>>>>>>>>>> InstallationLogger.generated-transfer-1653-1 >>>>>>>>>>> DEBUG 40% complete. >>>>>>>>>>> 2012-05-29 16:23:32,239 >>>>>>>>>>> InstallationLogger.generated-transfer-1653-1 >>>>>>>>>>> DEBUG 47% complete. >>>>>>>>>>> 2012-05-29 16:24:32,241 >>>>>>>>>>> InstallationLogger.generated-transfer-1653-1 >>>>>>>>>>> DEBUG 55% complete. >>>>>>>>>>> 2012-05-29 16:25:32,264 >>>>>>>>>>> InstallationLogger.generated-transfer-1653-1 >>>>>>>>>>> DEBUG 61% complete. >>>>>>>>>>> 2012-05-29 16:26:32,347 >>>>>>>>>>> InstallationLogger.generated-transfer-1653-1 >>>>>>>>>>> DEBUG 64% complete. >>>>>>>>>>> 2012-05-29 16:27:32,365 >>>>>>>>>>> InstallationLogger.generated-transfer-1653-1 >>>>>>>>>>> DEBUG 72% complete. >>>>>>>>>>> 2012-05-29 16:28:34,229 >>>>>>>>>>> InstallationLogger.generated-transfer-1653-1 >>>>>>>>>>> DEBUG 75% complete. >>>>>>>>>>> 2012-05-29 16:29:34,291 >>>>>>>>>>> InstallationLogger.generated-transfer-1653-1 >>>>>>>>>>> DEBUG 81% complete. >>>>>>>>>>> 2012-05-29 16:30:36,185 >>>>>>>>>>> InstallationLogger.generated-transfer-1653-1 >>>>>>>>>>> DEBUG 85% complete. >>>>>>>>>>> 2012-05-29 16:31:36,189 >>>>>>>>>>> InstallationLogger.generated-transfer-1653-1 >>>>>>>>>>> DEBUG 98% complete. >>>>>>>>>>> 2012-05-29 16:31:40,035 >>>>>>>>>>> InstallationLogger.generated-transfer-1653-1 >>>>>>>>>>> DEBUG Install Phase ... Done. >>>>>>>>>>> >>>>>>>>>>> TESTING: >>>>>>>>>>> - Confirmed that new logging works and appears in this new >>>>>>>>>>> format. >>>>>>>>>>> - Verified that the install still succeeds and the OS boots >>>>>>>>>>> correctly >>>>>>>>>>> after install >>>>>>>>>>> >>>>>>>>>>> Thanks, >>>>>>>>>>> Geoffrey >>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> _______________________________________________ >>>>>>>>>>> caiman-discuss mailing list >>>>>>>>>>> [email protected] >>>>>>>>>>> http://mail.opensolaris.org/mailman/listinfo/caiman-discuss >>>>>>>>>>> >>>>> >>>> >>> >> > _______________________________________________ caiman-discuss mailing list [email protected] http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
