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
