hey shawn, thanks for looking at this.
i've elided all the strait forward comments that i've simply addressed. replies to your other comments are inline below. i'm sending out an updated webrev in another email. thanks again, ed On Tue, Jun 05, 2012 at 04:43:59PM -0700, Shawn Walker wrote: > On 05/22/12 01:45, Edward Pilatowicz wrote: > >hey all, > > > >so i've finally got the parallel linked images code ready. > >you can find the webrev here: > > > > https://cr.opensolaris.org/action/browse/pkg/edp/pkg.pli.cr1/webrev/ > > > >the webrev contains a up-to-date design doc which is probably a good > >place to start. > > > ---------------------------------------- > src/client.py: > ---------------------------------------- > line 317: I don't think you need this assignment at all if there's > no usage text for the command. However, if it is needed (we should > fix that), you can at least drop the _() here. > i've removed the _(), but i don't think we want to default to "" for undefined entries here because that makes it too easy for someone to add a subcommand and forget to update the help messages. > line 6431-6432: This should not be doune outside of main_func or > handle_errors. If an exception happens while msg() is outputting > str(pkg_timer)),our internal exception handler won't catch it. If > you must do this here, then wrap it with a call to handle_errors. > i've wrapped the display call with handle_errors() > ---------------------------------------- > src/man/pkg.1: > ---------------------------------------- > line 337: s/the/the maximum/ ? Also, perhaps s/child > images/zones/ ? Realistically, we aren't going to be using this for > anything else and most users will likely stumble over "child > images". > > line 1987: Similar to 337, except s/number/maximum number/ ? > the man page changes have been removed from my putback and will be fixed via 7171861. i worked with alta to come up with the following text: -C n Specify the number of child images to update in parallel. When recursing into child images (usually zones), update at most n child images in parallel. The default number of child images to update in parallel is 1. If n is 0 or a negative number, all child images are updated in parallel. See also PKG_CONCURRENCY in the "Environment Variables" section. ... PKG_CONCURRENCY The number of child images to update in parallel. Ignored if the -C option is specified. When recursing into child images (usually zones), update at most $PKG_CONCURRENCY child images in parallel. If $PKG_CONCURRENCY is 0 or a negative number, all child images are updated in parallel. Default value: 1 > ---------------------------------------- > src/modules/actions/license.py: > ---------------------------------------- > line 78: there's no comment explaining this, but my guess is that > this is a side-effect of plan serialisation. What was the reasoning > behind this particular approach? > the call to preinstall() is necessary to initialize "path" in the case when we've re-loaded a previously prepared plan. (otherwise during execution "path" may not be initialized and we'll tip over.) i've added a comment. > ---------------------------------------- > src/modules/client/__init__.py > ---------------------------------------- > line 61: used by whom or what? > updated comment: # runid, used by the pkg.1 client and the linked image # subsystem when when generating temporary files. > ---------------------------------------- > src/modules/client/api.py > ---------------------------------------- > line 380: can you explain why this change is needed? the comment > should. I purposefully deferred the loading of the catalog until > needed for performance / memory usage reasons. > at this point i don't remember exactly why i did this. i'll remove and and re-do my testing to see if it was really necessary. > line 1289: I don't like exposing pubcheck as part of the general API; > is there a way to have this automatically determined so we don't > have to expose an implementation detail to callers? > unfortunately not. we could get rid of it by documenting that all api consumers manually perform a publisher check before creating an image plan... but that requires updating all api consumers. i've added the following docstring for this paramter: 'pubcheck' indicates that we should skip the child image publisher check before creating a plan for this image. only pkg.1 should use this parateter, other callers should never specify it. > line 1290: I can't find where "show_licenses" is actually used, and > I'm also confused as to what purpose it serves. If it's similar > in purpose to 'accept', it needs a docstring, and now we have an > inconsistency in naming for 'accept'. > as we discussed in person, this has been removed. > ---------------------------------------- > src/modules/client/imageplan.py > ---------------------------------------- > line 116: This implies that we always go through plan > serialisation; that's unexpected. Please ensure that we don't go > through plan serialisation unless linked images are involved. > as we discussed, we don't always load a plan. i've renamed this to setup_plan() > ---------------------------------------- > src/modules/client/pkgremote.py: > ---------------------------------------- > lines 177-182, 231-236, 444-446, 469-471, 491-494, 503-508, 559-562: > seems like these need to be try/finally to ensure lock gets > released even if execution is interrupted > i don't think so. first, the locks are all in memory process locks. second, all the code under locks has been written such that the only way it will fail is due to a bug. almost all the code under locks is actually simple variable assignments (the exceptions are Thread.start(), assert calls, and debug messages if they are enabled). if any of these operations fail we are not going to recover and the client is going to die with a stack trace and exit, there by making process lock recovery irrelevant. > Someone else should review the threading code here too. > i'll harass some other folks to make sure i get more eyes on this code. :) > ---------------------------------------- > src/modules/client/plandesc.py > ---------------------------------------- > lines 421, 477, 501: why do we have a method that just simply > returns a property value? > these were all pre-existing methods on the current PlanDescription object. i was keeping them around for compatability. but i've gone ahead and removed them. > ---------------------------------------- > src/modules/misc2.py > ---------------------------------------- > Not thrilled about having a separate module only because of > pylinting :-( > i've gone ahead and folded misc2.py back into misc.py (this required cleaning up some more pylint issues in misc.py) > lines 250, 514: I don't see where 'type' gets assigned, and I > don't think we should have a variable named the same as a builtin > function either. > 'type' doesn't get assigned anywhere. this is a reference to the builtin. > ---------------------------------------- > src/modules/prstart.c > ---------------------------------------- > Weird to have a whole module just for this function > carved out of the namespace. > i didn't think the module namespace was a scarce resource. i'm open to suggestion for putting this functionality into an existing module. > The timings stuff also seems off somehow. For example, running 'pkg > update -nv' on my system takes a total of 7.00s user, but timings > reports a total of 0.217s user. It looks like it's only timing > client startup. > could you include the output from the command? i've been doing some runs with timing enabled and the results seem ok. _______________________________________________ pkg-discuss mailing list [email protected] http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
