Brock,
On Mon, Sep 28, 2009 at 02:24:37PM -0700, Brock Pytlik wrote:
> Webrev:
> http://cr.opensolaris.org/~bpytlik/ips-7960-v1/
transport.py:
- lines 456-460: Why is this exception handler necessary?
- lines 669: This check is different from the previous conditional.
Is it possible for cache_path not to exist, even if lookup returns a
path.
modules/file_layout:
- The source under this directory hierarchy is < 300 lines of code.
Could we coalesce this into a single file in modules/ or am I
missing something?
file_layout/__init__.py:
- lines 29-31: This means that if you import file_layout without
further arguments, you'll get everything in file_manager and layout.
Is this what you intended? You're generally importing these modules
specificially when you need them in other code.
file_layout/file_manager.py:
- line 33: How about, "This exception is raised when the caller
attempts to modify a read-only FileManager."
- lines 40-42: This might be more clear as, "The FileManager cannot
create %s because it is configured read-only."
- line 46: This name conflicts with a class in another module:
api_errors.PermissionsException.
- General comment about these two exceptions: Should these inherit
from some form of API exception? Unless you're catching these in
every code path that might emit them, it probably makes sense to
give the API some kind of ability to catch these.
- lines 72-73: Should this be a class property, it doesn't look like
it's going to vary between instances?
- line 91: How does this function cope with files that may be
readable in one tree, but not the other? If you've found a file
that exists in both layouts, but is only readable (or modifyable) in
one, is there any mechanism to ensure that this works properly?
What do you do if you can't move the file from the old layout to the
new layout? It looks like you have some code on lines 132-134 that
tries to handle this, but it doesn't appear to make a distinction
between the move failing because the destination wasn't writable,
and the move failing because the source wasn't writable. I can
imagine some cases where we'd want to copy the file over instead,
even if we can't remove it.
In fact, if you had a depot on a stick, you might want the layout
manager to copy the files over to a local disk as they're accessed,
but this current model doesn't look like it would support that kind
of incremental depot migration.
- lines 147-149: Why not defer the lookup on 146 until we've checked
read-only?
- lines 182-184: Same comment as above.
file_layout/layout.py:
- This module needs more documentation.
- Why do you allow Original to name itself "s1" by defining SHA1_160,
but Fan256Once allows the class that instantiates it to give its
name. Wouldn't it be better to encode the name as a class property
in both cases instead?
- lines 59/60 and 65/66: These should match the function signatures
in the parent class that they are overriding.
General comments:
- You did this work as part of a performance investigation surrounding
file layouts and directory access times. It would help the rest of
us provide useful feedback if you would include information about
the experiments that you ran, and the data that you obtained. In
particular, how did you decide that one level of 256 directories was
optimal?
- You've created the file layout and file manager as part of an
extensible architecture. Would you provide more information about
your design and architecture? As an example, how would I go about
adding a new layout to this design?
- The FileManager implements a cache of sorts, but I don't see any
hooks for some of the typical operations that would be performed on
a cache. How do you determine what items to evict when the cache is
full? How do you plan to support multiple policies, both for sizing
and eviction? Are there any options for those who need to perform
external cache maintenance?
-j
_______________________________________________
pkg-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
