Hey Brock, I took a look at your webrev. You've written enough new code that I'm probably not able to review it as thoroughly as I can with the existing code. It would be great if you could write up a description of your design, and how the different parts work. That may give some of us who aren't experts in search issues a chance to provide overall feedback on the design.
> Here's the webrev for the new search backend: > http://cr.opensolaris.org/~bpytlik/ips-search/ Here's some specific feedback that I've come up with so far: client.py: - line 399-416, 500-517, 1432-1451: You've duplicated a lot of the error handling code. Would it make sense to commonize this in the top-level exception handlers that we use to wrap main_func()? Meta-question here too: Some of these errors instruct the user to remove directories and then run, or re-run commands. Would it make more sense to: a) Provide a command that repairs the indexes without any further user intervetion b) If yes to a, then can we just run the repair when we encounter an error instead of returning a message? depot.py: - line 150: Can we use a lockfile insead of a scary warning here? pkg.1.txt: - I would omit "should be generally unnecessary, but" catalog.py: - line 43: The catalog code is shared between the client and the server. Adding this import means that the client now depends upon the server for correct functionality. I don't think we want to do this. - line 147: This suggests, perhaps, that we want a subclass of Catalog for the server? - line 208 & 209: Is there a reason why the tempfile needs to have the same name for every invocation? Would it be better to use tempfile.mkstemp()? - line 220: Shouldn't we drop the lock here? - line 249: The hold time on this lock looks long. Who's contending for this? - line 349: Who releases searchdb_lock? More importantly, what's the lock ordering for the update lock and searchdb lock? This should be documented. You should also document what each lock protects, preferably in the constructor, or somewhere conspicuous. - line 376: Shouldn't we drop the update and searchdb locks here? - line 377: Why don't we drop the updatelock for non-posix systems? indexer.py: - line 105: It seems like it would make more sense for the Indexer to be able to determine it's max memory use when it's created. You've duplicated the max memory calculation in image and imageplan. I would either have __init__ figure this out, or pass __init__ a function that it can call to figure this out. - line 658 / 669: Why do we need separate update index functions for the server and the client? misc.py: - line 328,329: Shouldn't we determine these dynamically, or at a minimum not let this number exceed the amount of memory in the machine? search_storage.py: - lines 492-495: Remove dead code, please. Thanks, -j _______________________________________________ pkg-discuss mailing list [email protected] http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
