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?
- 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
