Jean,
Responses below. I removed items where I had no further comments.
It sounds like the data restructuring will help a lot, which is fantastic.
- Keith
On 10/12/10 10:57 AM, jean.mccormack wrote:
Keith,
Thanks for the in depth review. Lots of good comments and I learned
yet more about some of the python calls.
Responses are inline. Note: Talking with Drew after his review yielded
some restructuring that I reference here.
Basically the method of storing and retrieving attributes relative on
a per transfer basis is being overhauled for the better
with a huge reduction in complexity and code size.
Ginnie will respond with the items I can't answer.
Jean
On 10/11/10 06:29 PM, Keith Mitchell wrote:
[...]
system-library-install.mf:
Should this actually be in system-install.mf? That's where the
current transfer module is.
I think that all of the checkpoints should go the same place. DOC and
MP are in system-install-library so that's where transfer should then go.
Ok. I understand that perhaps those 2 packages will be re-evaluated in
the future, so I think that's fine then.
transfer_cpio.py:
64: This variable might be more flexible as a tuple of ("-p", "-d",
"-u", "-m")
Not really. The design spec calls for just passing args through
without any processing. So holding it as a string is in fact easier.
This works for the current set of arguments, but if we ever want to
change the default args to use an option that requires an argument
(e.g., "-C bufsize"), it will be easier to do so if this variable is a
list/tuple - since subprocess.Popen/call/check_call takes a list of
arguments, it'll be looking for something of the form:
["cpio", "-i", "-C", "16384", "-p", "-du", "-m"]
"-pdum" works currently because cpio can handle that as a single
argument. (Even if this were changed to be a single item list,
["-pdum"], it leaves the door more flexible for future changes).
99-107: It seems to me that this scenario might be better handled by
something akin to the following:
except SourceNotExistError:
# _parse_input should raise this for cases such as "self.src and
not os.path.exists(self.src)"
return self.DEFAULT_SIZE
Note that the 'else' clause is purposely left out - the
TransferUnknError raised here can't provide a better reason for the
failure, so there's no reason not to simply let the original
exception propagate.
Agreed.
119: Use "partition" instead of "split" (partition won't cause an
error if there happens to be a line in the file not of the form
"XXX=YYY" at some point in the future)
Will do.
115-124: I think this should be done prior to the 'for' loop
Agreed. In fact, Drew and Ginnie and I were talking about some
refactoring of the code and when looking at that I found this issue.
Nice catch.
155: Nit: Would it make sense to use self._cancel_event for
consistency with other checkpoints that don't define a custom
cancel() method?
Instead of self.abort? I'm fine with that.
159: I'm not sure what the reason is for this method - can you
explain? Will other checkpoints feasibly need something similar
(i.e., should there be something more global in the engine or logging
module)?
If the user had not called the get_progress_estimate function then
reporting progress is stupid. So give_progress is set when they call
it and progress logging only occurs in that case.
Will other checkpoints need something similar? Possibly.
My line of thought here is that, if get_progress_estimate hasn't been
called, there probably aren't any ProgressHandlers assigned to the
logging module. In which case, calls to report_progress() are hopefully
being filtered out appropriately.
In other words, the logging module is designed to perform filtering of
logging calls based on logging level, engine status, what handlers
exist, etc. I don't think the checkpoints should have to worry about it
- they should just call "report_progress", and if the logging module
determines that there's nowhere to send the message, then it just gets
ignored - just like if the logging level is set globally to
"logging.WARN", logging messages at the debug level are simply ignored
by the logger.
184, 188: General: When logging from except clauses, use
logger.exception() rather than logger.error/critical, as
logger.exception automatically dumps traceback data for the current
exception. However, see next comment
183-189: I think it might be best to simply propagate the error
(putting cleanup steps in a 'finally' clause and removing both
'except' clauses). The engine will take care of logging the uncaught
exception. In particular, re-wrapping the error on line 189 causes
the loss of useful data from the original exception, and I don't see
any value added specifically by the TransferUnknError here.
I don't have any issues doing this, but awhile ago we had talked about
checkpoints only returning known errors. i.e. some type of Transfer
Error in this case.
So if you're going to catch all exceptions I'm fine with it.
[...]
291, elsewhere: I think Drew had a good suggestion here, but given
the behavior of os.path.join, this could simply be:
fname = os.path.join(self.src, file_list)
os.path.join, upon encountering any arg that is an absolute path,
disregards all prior arguments, so there's no need to if/else this.
Really? You know I'll have to test this, super cool if it does. And if
so, will change. Thanks.
Yup, really. It's both a blessing and a pain, depending on if you're
aware of that behavior when you try to use the function...
[...]
667: Would rather not see yet another "run/exec/cmd" function in our
source code. Given the current single use case for this function,
simply embed the check_call in _transfer - would even be fine without
the try/except clause, I think.
I'd rather leave this as a method in case it needs some beefing up.
If a special wrapper for subprocess.call/check_call/Popen is really
needed, it should probably be implemented in a way that's usable across
the entire code base. This is one of my major pet peeves in our
slim_source code - every module seems to have 1-3 special wrappers for
executing a subprocess. Last time I counted, we had about 8 separate
methods for this, most of which do nearly the same thing.
https://defect.opensolaris.org/bz/show_bug.cgi?id=15957
http://opensolaris.org/jive/message.jspa?messageID=474361#474361
Personally, I don't think we need any wrapper around the subprocess
functions; but even if we do, 1 or 2 at most seems like it would be
enough. I don't think we need 8, 9, or more.
If embedding the subprocess.check_call() call is truly not desired here,
would it at least be possible to use one of the existing wrappers,
rather than rolling up a new one?
704: Use "self.logger.info("Transferring files to %s", self.dst)"
As a general rule, for logging statements, use formatting string
operations and pass in the formatting arguments as separate
parameters. This allows the logging module to defer the (relatively
time intensive) string formatting operation until it determines if
the logging call will actually occur or not (based on the log levels).
While it's only really important for code that is run frequently (in
a loop processing a large amount of data, for example), it's a good
practice to get into.
Will do.
754-760, 960-966: These are init'ed in AbstractCPIO - is there a
reason to reset them here?
They are actually going away. But there is/was a reason. I wanted a
clean attribute every time parse_input is called.
transfer_err.py:
Philosophically speaking, i don't see a need for these exception
classes. Uses of "TransferValueError" should definitely be directly
replaced with the basic Python ValueError, which callers would
reasonably expect from a function if they pass in invalid parameters.
Which is how it was originally but was changed after the big talk
about exceptions and ( to be honest) a comment you made at one point
about ValueError.
Hrm. I find exceptions to be tricky... I've waffled in my philosophies
on them at times, that much I know.
The rest of the exceptions defined here seem to indicate *where* the
exception came from, rather than *what* the exception was - the
latter being far more valuable to a caller or end-user. The engine
already knows what checkpoint is running - and the stacktrace data
from an exception will further make that clear. In terms of resolving
an issue - either finding a programming bug that raised an exception,
presenting adequate information to the user to fix a permissions
issue, or programmatically recovering from an expected error case -
it's easier to use an exception that matches what happened (IOError -
bad permissions / file not found; TypeError - Function can't handle
objects of type XYZ; etc.).
This all goes back to a discussion at one point about having the
checkpoints return exceptions they have defined. If we have gotten
away from that, fine.
But that is probably a larger architectural decision.
I think I recall that discussion - it was in reference, initially, to
ManifestParser/Writer, as I recall. My memory may be deceiving me, but I
think my stance on that was that I would have preferred to have the
original exceptions from lxml propagated, but (grudgingly) admitted that
for MP/MW it was acceptable to wrap the exceptions, so long as the
original exception was stored and available for examination.
[...]
58-59: The "SOFTWARE_" prepended is redundant - just use "LABEL" and
"NAME_LABEL"
Ditto for the equivalent items in Destination and Dir classes.
That is again something that needs to be decided amongst all of the
checkpoints. MP did it this way. I followed their convention because
consistency is good.
Ok. I'm sometimes inconsistent with whether or not I catch these things
(or for that matter, whether or not I care about them). So for
consistency's sake, leave it in.
[...]
604-639: These lists can be stored in memory, right? Would it be
better to do so? A temporary file seems prone to issues across
pause/resume, particularly if the system is rebooted in between.
Not really. We are passing things through the DOC and I'm not sure we
really want to carry what could be 1000's of entries around. Doesn't
pause/resume
reread the manifest?
Out-of-process resume will only reread the manifest if the app chooses
to do so. For DC, this is done, since the user may modify the manifest
in between. For other apps, this may not be the case.
If there's concern about passing 1000s of entries around, it may be
necessary to look into defining whatever special functions are used by
the pickle module to load/unload an object, so that when this object is
serialized, the entries from the file are stored in the DOC snapshot as
well.
[...]
transfer_ips.py:
73: My understanding of gettext is that gettext.install should *only*
be called by an application, never by a module - modules should use
something like:
_ = gettext.translation
gettext.install() modifies the global namespace, so running it will
affect all imported modules - which means this line here will
override, or be overridden by, the call to gettext.install() that
will come from DC.
Similarly, I don't think a checkpoint should set locale.LC_ALL globally.
But the IPS calls don't work without this code. ;-(
We'll need to work through this to make sure that our modules and pkg's
can be localized properly in the same process-space. The pkg.client code
appears* to be violating some of the rules set out by the gettext module
for use in libraries:
http://docs.python.org/library/gettext.html#localizing-your-module
* I say appears, because I'm really not sure. Localization is something
that eludes my grasp quite frequently, and it's all to easy to blame
someone else's code for the problem, but I don't think it should be
necessary for a consumer of an API to have to work around the
localization code used within that module.
I *suspect* that use of gettext.bindtextdomain("pkg",
"/usr/share/locale") will make their code work. If adding that call at
the top level of the module causes the pkg.client code to run, then the
best approach is probably a context manager that sets the text domain to
"pkg" before each pkg.client call, and resets it afterwards.
If this isn't easily resolvable, the best approach is probably to file 2
bugs - 1 against pkg.client, and 1 against this code, and try to work
together to figure out what needs to be done in order to make both sides
localizable in a way that neither side interferes with the other.
(Actually, filing those bugs may be needed anyway - even if the
bindtextdomain method works, in a perfect world that wouldn't be necessary).
[...]
134: Can you explain what this math is doing here? It appears to be
that each pkg is assumed to be "roughly" the same size based on a
pre-generated average, and the number of packages given is correlated
against that?
Yes. Eventually we'd like to get package size information from IPS but
it's not there yet. Then this would be far more accurate of a
calculation.
Got it, that makes sense. Will there be a bug filed to update this code
when the relevant pkg API is available?
139-140: Since get_size() appears to only be used here, could
get_size/distro_size be combined into a single @property called
distro_size? Use an internal self._distro_size variable to cache the
results of the work currently done in get_size(); if the cache is
set, return its value; if not, calculate it, set it, and then return
the value.
Possibly. But get_size is part of the defined interface so it's not
only used here.
Ah, got it. Fine as is then.
215-219: Use set operations:
not_allowed = set(["prefix", "repo_uri", "origins", "mirrors"])
image_args = set(self.image_args)
overlap = a & b:
if overlap:
# error
217-219: Malformed format string
Are you sure? I've actually seen this work I believe.
Assuming not_allowed = "repo_uri", the output here will end up being
literally:
"%s to use should be repo_urispecified in the Source, not the args"
i.e., the string will be concatenated, not formatted. I think what you
want is:
raise ValueError("%s to use should be specified in the Source, not the
args" % not_allowed)
If that needs to line wrap, use implicit concatenation - without a "+",
i.e.:
raise ValueError("%s to use should be "
"specified in the source, "
"not the args" % not_allowed)
222-3: Quick nit:
self.prog_tracker = self.img_args.get("progtrack",
self.DEF_PROG_TRACKER)
Should achieve what you're going for, I believe.
In which case I wouldn't have to set self.prog_tracker to the default
at some point.
Right, self.prog_tracker could simply be initialized to None in __init__
(or left initialized to DEF_PROG_TRACKER - though it needs to be
initialized to something to appease Pylint and to aid things like Pydoc
that use __init__ to determine what the variables of a class "should" be).
228: A recent change in pkg(5) made it so that publishers with
installed packages could never be removed - removing them would
actually simply *disable* them. Will that cause problems with this
approach, i.e., removing an existing publisher on line 310, then
adding it back in (with potentially different origin/mirrors) on line
320/324?
I think we're ok. Remember that email thread?
I do, now that you mention it, and yeah, it should work fine as is.
Sometimes I spook to easily...
[...]
394: pop() is a function, so the arg should be in parentheses, not
brackets. pop() also removes the item from the dictionary, which may
or may not be desired (e.g. if something fails, but it's recoverable,
and this code executes again, "recursive_removal" will be gone - it
may be desirable to instead create a copy of args and use that for
this section of code)
Thanks for catching the brackets. Removal is the desired action in
this case but the cases you expand upon are definitely ones to be
thought about.
An empty dictionary will work just fine with **, so if args is always
a dictionary (or possibly None - in which case, add an "if args is
None: args = {}" chunk of code somewhere), then 391-401 can be
combined into:
args may be None. So it comes down to is it nicer to have the if else
or the args = {}. 6 of one half dozen of another in my mind.
recursive = args.pop("recursive_removal", False)
self.api_inst.plan_uninstall(pkg_uninstall, recursive, **args)
438-439: Code can be removed
Yup. Obviously there used to be more code that was removed.
Yup. I've done the same many times...
451: Will stacktrace if self._origin is None. (some_list[1:] will
never be None - if there are no items there, it will return an empty
list). line 451 could probably just be removed, and line 452 could be
part of the 'if' block from line 449.
I think you can't do that. If I move 452 to follow 449 then if
self._origin[1:] returns an empty list, I have set origins to an empty
list which isn't what I want. I don't want to set it at all in that case.
In that case, the line should be "if self._origin[1:]:". Don't compare
to "is None", because a list slice will never be None - it may be empty
though.
Actually, I know _origin is not None. It's at worst and empty list
since it's initialized to that. I think the error is really at line
449 which should be
if self._origin[0] is not None:
[...]
507: I don't know if this message will make sense to an end user -
how would they end up in this situation, and how might they resolve
it? The API Errors from pkg.client.api_errors seem fairly verbose - I
think it would be possible to simply propagate them, unless we have
additional context to add to the message, or have a way to recover
from the exception.
It certainly made sense to Drew and Alok. They end up in this
situation if the pkg version is different from our version. I believe
their errors weren't quite as good in this case. Or they didn't really
help Drew and Alok when they saw them.
Ok. From a developer's standpoint, "API version specified" makes sense.
I'm just thinking that someone unfamiliar with IPS internals the way we
are might see that message and have no idea what they did wrong or how
to fix it. If this is an issue that will only ever be seen by developers
(i.e., we'll catch API version changes and resolve them before this
reaches an end user), then no need to worry about it. And I think that's
the case, so carry on, nothing to see here.
569: What is this clause catching? The message seems rather specific
to a failure at line 563, and I imagine it could be constrained to a
single exception class.
Either 563 or 565 or 566. If any of these raise an exception the
destination hasn't been specified correctly.
Sorry, I wasn't clear on that. What exception types are we catching
here? They should be listed explicitly, rather than having a bare
'except' clause.
575-578: If im_type.zone is a bool, simply set "self.is_zone =
im_type.zone". If it's something else, use "self.is_zone =
bool(im_type.zone)"
My C background is showing here. Will change.
642: Nit: Initialize value_str to an empty string, and lines 646-648
aren't needed.
This code is moving to DOC so you'll need to consult with Darren on this.
Ok, forwarding it on to Darren in a separate email.
712, 726: A more specific class than "Exception" would be preferred
here.
Yeah.
683: In looking at the complexity of the branching, I wonder if a lot
of the publisher related code, here and elsewhere, would be
simplified by having a single list of publishers, stored in "search"
order. The first item in the list would be what's known as the
"preferred" publisher, and the rest are additional publishers, in
order of precedence. This would seem to map more naturally to how the
publishers on a system are laid out these days (preferred is nothing
more than a way to say "search this publisher first").
kemit...@kemobile-work->~ 0 $ pkg publisher
PUBLISHER TYPE STATUS URI
opensolaris.org (non-sticky, preferred) origin online
http://ipkg.sfbay/dev/
contrib origin online
http://pkg.opensolaris.org/contrib/
extra origin online
http://ipkg.sfbay/extra/
kemit...@kemobile-work->~ 0 $ pfexec pkg set-publisher
--search-before=opensolaris.org extra
kemit...@kemobile-work->~ 0 $ pkg publisher
PUBLISHER TYPE STATUS URI
extra (preferred) origin online
http://ipkg.sfbay/extra/
opensolaris.org (non-sticky) origin online
http://ipkg.sfbay/dev/
contrib origin online
http://pkg.opensolaris.org/contrib/
Actually, it's not too different. We start out with publishers stored
in order, first being preferred. Unfortunately preferred and rest are
set in IPS via very different mechanisms so it was easier to break
them out for internal storage.
Fair enough. I wasn't sure how the pkg.client.api differed from the CLI
in how this is handled, so I was mostly making a blind suggestion there.
745: Nit: The "+" and the "\" at the end aren't strictly necessary
(The "\" is implied since there is an unclosed parentheses; the "+"
is not required for concatenation of inline string constants)
(Saw this in a few places, truly not a big deal, just thought I'd
point it out for future reference)
837-841: Nit: Enclose in an "if
self.logger.isEnabledFor(logging.DEBUG):" clause, for reasons similar
to prior logging comments.
So you're implying that if all the code does is log something to
enclose it? Reason being? Seems to me that all you save is the code
not being executed but at the expense of readability. And lots of
other logger.debug statements are being executed.
Yeah, that was me being overzealous again. This scenario wasn't needed.
In a hypothetical scenario like the one below, you'd want to enclose the
call in such an "isEnabledFor" clause, to avoid doing unnecessary
processing, but this code isn't actually processing anything so there's
no need here.
if self.logger.isEnabledFor(logging.DEBUG):
some_information = self.process_lots_of_information()
# takes a few seconds to run, so don't run it if we're not going to
end up logging it anyway
self.logger.debug("Some information:\n%s", some_information)
869-871: Use "elif"?
Sure.
881: Initializing self.args to an empty dictionary would remove the
need for this type of logic.
Yes. Will look to make sure this doesn't break something else first
though.
Understood.
transfer_p5i.py:
77-82: Remove try/except clause, and let exceptions propagate?
Sure.
67, 87: You've popped the first publisher twice - was that intended?
Yes. In this case, the first publisher is the p5i file and the 2nd is
the preferred and I want both of those dealt with and only additional
ones left.
Ok. When I reread the code and the comments again, that makes a bit more
sense. It may or may not be worth adding a comment around 67, or in a
docstring, about the structure of pub_list.
transfer_prog.py:
[...]
101: When would this occur?
The df code (on tree usr/src/cmd/fs.d/df.c allows for the case where
-1 could be there.
Would it be better to raise the error after line 81 then?
103: This looks odd as well - should the second equals sign be a
minus sign?
Yup. See above.
118: Could this be moved up to line 98 as the condition on the
"while" statement?
The first time through you don't have pct yet so it would involve more
work than I think is necessary.
Ok.
[...]
103-133, 162-190: This seems to overlap with the same functions in
prior files; would a common meta-class be worthwhile, defining these
basic things?
Possibly? So you're thinking a class like TransferClass which
TransferCPIO, TransferIPS...... are all children of? And they would
inherit some of these methods from?
Yes, something like that. Very lightweight, minimal number of methods
and a small set of common attributes.
[...]
_______________________________________________
caiman-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
