On 08/ 9/10 04:20 PM, [email protected] wrote:
Hey Shawn,
On Mon, Aug 09, 2010 at 01:03:32AM -0700, Shawn Walker wrote:
webrev:
http://cr.opensolaris.org/~swalker/pkg-repo-format/
...
image.py:
- line 56: Do you know how the code on line 210 worked before this
import was present? Were we getting this through another module,
just by accident?
No idea how. My guess is that it was somehow being imported into the
pkg namespace already.
- The NRLock (and RLock) classes have an _is_owned() method. It
would be simpler to remove __locked entirely, and just call
__lock._is_owned() instead.
_is_owned is a protected interface that external consumers aren't
supposed to be using. However, per our discussion I'll add the
equivalent the NRLock class and use that instead of my own thing.
...
transport/transport.py:
- publish_rebuild(), publish_rebuild_indexes(),
publish_rebuild_packages(), publish_refresh(),
publish_refresh_packages(): I would prefer it if these functions
called __gen_repo() with "admin", [0] and raised an exception if the
repository does not support the requested operation.
Per our discussion, I'm going to change __gen_repo to raise
UnsupportedRepositoryOperation if an operation and versions are
provided. That should handle this and remove various duplicates we have
for this logic in other places.
modules/indexer.py:
...
- I also wonder if it's possible to take the common file locking code
from image and indexer, put it in its own module, and make these
common functions. This seemes like it would be useful, instead of
copying the code around.
Unfortunately, the exception handling and the exceptions raised are
slightly different between each implementation. I don't think there's
much I can reasonably commonise without making it difficult to understand.
modules/depot.py:
- line 195: Was there a particular reason why you chose Lock()
instead of NRLock here?
No; I'll change to use NRLock.
- line 1013: Would it be possible to make the Queue larger, and
simply place a limit on how many of each operation may occur in
parallel? That is to say, if you're already running a
rebuild-index, and you get a request for a second rebuild-index,
could we just accept the second and ignore it instead of returning
an error?
It's intentional that only one process be queued up at a time. Since
almost every operation that would be placed in the queue is long running
and because some operations might conflict with each other, I wanted it
to be setup so that you could only queue up a single operation at a time
so that you didn't end up with (say) 20 index operations all waiting to
be processed ;)
- line 1120: What is the purpose of this operation? I read the
docstring, but I'm still not clear on what this is actually used
for.
Mismerge or leftover development code; it's gone now.
- BackgroundTaskPlugin: You have a potential deadlock in the stop
path. If thread A sets __running to False and waits for thread B to
exit by calling join(), and thread B exits, but then thread C calls
put(), thread A can block forever in __q.join() since no thread will
be servicing the queue at this point.
I would call __q.join() before __thread.join(), and prevent tasks
from being put() once stop() has been called. If you have stop()
post a final task to the queue, to wake up the sleeping thread, you
can eliminate the timeout kwarg from the __q.get() in run().
Per our discussion, I'll simply be removing the join bits in stop()
since I don't care if waiting tasks get processed or not.
server/repository.py:
...
- line 672: Should this be a finally, so that the index is always
unlocked?
Yes; thanks.
publish.py:
- lines 177 and 178: Not trying to stir up trouble, but should we
print a warning to stderr that --no-index is not supported any
longer instead? We can remove that warning later on.
I don't think it's really necessary. If you specify --no-index, you're
getting the behaviour you asked for implicitly ;)
It's actually the opposite case where a warning would seem to make more
sense. I don't think a warning will be sufficient to stop people from
using the option if that's the intent.
I'll await further review comments before posting a webrev.
Thanks!
-Shawn
_______________________________________________
pkg-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
