Hi Geoffrey,
I totally forgot to respond to you on this sooner, sorry.
Taking a look at this, I'm primarily happy with the code-as-is now,
but with a couple of nits:
- You are importing timedelta but not using it - I know that the
comparison of the times is generating one, but you don't really need to
import it.
This should be flagged with pylint, so have you run pylint and PEP8
on the code?
- Where you are outputting the percentage, I think it would look better
if that include the action that is occurring, e.g. rather than:
Install Phase ... Started.
24% complete.
...
98% complete.
Install Phase ... Done.
you would get:
Install Phase ... Started.
Install Phase ... 24% complete.
...
Install Phase ... 98% complete.
Install Phase ... Done.
the reason being that the console only has 25 lines, and if things
are taking long enough that over 25 minutes have passed (who knows,
it could happen) then it wouldn't be clear what phase is at 98%
complete :)
Thanks,
Darren.
On 05/06/2012 16: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
