[email protected] wrote:
Brock,
As requested, I've taken a look a look at your code review. Comments
are below.
http://cr.opensolaris.org/~bpytlik/ips-2670-v1/
client.py
- line 1035: Is there a reason why we don't automatically rebuild the
index if we know it has been corrupted?
Basically, pkg search is supposed to be a fast operation, reindexing is
a comparatively slow operation. I thought it was better to let the user
choose when to rebuild their index rather than doing it automatically. I
don't believe this is a change in functionality, so if you think it
should be changed I'm happy to consider it for another changeset.
directory.py
- line 51: use os.path.sep instead of "/"
file.py
- line 61: use os.path.sep instead of "/"
link.py:
- line 50: use os.path.sep instead of "/"
Ok,, I'll fix those
actions/[directory | file | link]:
- In each of these __init__() routines, you perform a re.search. What's
the performance impact of performing a regular expression operation
each time a file/directory/link action is created? Can you compile
this re, instead of creating it anew each time? Is there another
faster way of performing this check?
Well, since we're only raising them when we're raising a
MalformedActionError, I thought the performance wouldn't really be an
issue. My impression is that, generally, when we hit one of those, we're
going to come to a halt. If it's truly a concern that we're going to be
doing this thousands of times, I can look deeper into this.
api.py:
- Is there any reason why all the calls to __reset_unlock() can't go
in the finally clause where the activity lock is released?
Because finally clauses are executed whether or not an exception was
thrown. Right now, we only make the call to __reset_unlock() if an
exception is thrown during execution. We could add another level of
try/except so that all of those happen in one statement, but I'd rather
not do that in this changeset as I think it will mean making the already
hard to read code even tougher as the lines will be starting another
characters to the right.
- Can you move remote_search() to client/retrieve.py? Is there any
reason why it has to be in the api.py module?
api_errors.py:
- line 325: You're duplicating code that's elsewhere in the system.
Check out TransportFailures in misc.py. If need be, we should
promote some transport exceptions to ApiExceptions. IIRC, Shawn did
some of this with his most recent putback. Keep in mind that I'm
reserving the right to change any TransportExceptions in a future
putback. However, it seems unwise to have multiple exceptions
handling the same problem littered throughout the code.
I'll take a look at how hard it would be to move this into retrieve.py,
mirror the error handling code there, and replace the code in api.py
with a stub. For now I'm inclined to leave it in place, and look at
refactoring it when there's slightly less time pressure.
client/query_parser.py:
- line 78: Is the granularlity of this lock correct? This is a
class variable. All instances of TermQuery will use the same lock.
Is it possible that different instances of TermQuery will be
accessing different datasets? If so, they'll serialize unncessarily
against a lock, even when they're referencing different datasets.
Yes, the lock is supposed to be at the class level. Within a single
process, all instances of TermQuery will be using the same datasets.
This lock is here to allow the reuse of those datasets rather than
having to reread them from disk every time. I can elaborate more if this
isn't clear.
indexer.py:
- It looks like you need some I/O exception handling. The
__close_sort_fh(), _add_terms(), _write_main_dict_line(),
_generic_update_index(), _setup(), and _migrate() all look like they
need some kind of EnvironmentError handling. I'm assuming we don't
want to crash with a stack trace if files/directories get errors
here.
J and I talked and we agreed that this should be added at some point,
but for the first build or two, if this code stack traces, having those
stack traces may be very helpful in figuring out what's wrong. In short,
I plan to fix this for the release, but not in this putback.
query_parser.py:
- Would you please add more comments and docstrings to routines in this file?
Sure
depot.py
- lines 294-296: Remove XXX and code that has been commented-out
Thanks, already gone :)
Thanks a bunch for taking a look at this and getting back to me so quickly.
Brock
-j
_______________________________________________
pkg-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
