[EMAIL PROTECTED] wrote: > Folks, > > Here's a code review for the initial set of mirroring support for pkg > depots. We'll enhance this more over time. > > For now, the general idea is that mirrors are only able to serve package > content; whereas a full depot may serve metadata, and provide other > services. (Search, etc.) > > Webrev is available on cr.opensolaris: > > http://cr.opensolaris.org/~johansen/pkg-mirror1/
http://cr.opensolaris.org/~johansen/pkg-mirror1/src/client.py.wdiff.html ========== 105 + [-O origin_url] [-m mirror to add] [-M mirror to remove] authority I don't think -m and -M are very obvious -- they don't say to me as a user, "add and remove." Specifically, it would be too easy to say -M when I met -m or vice-versa. Could we use --add-mirror and --remove-mirror instead or some other set of letters like -a and -r? 1622 1658 except misc.TransferTimedOutException: 1623 1659 msg(_("Maximum number of timeouts exceeded during download.")) 1624 1660 sys.exit(1) 1661 + except misc.InvalidContentException, e: 1662 + msg(_("One or more hosts providing content for this install " + 1663 + "has provided a file with invalid content.")) 1664 + msg(_(str(e))) 1665 + sys.exit(1) Would it be better to use emsg() for these instead of msg() so that they go to stderr? What is our rule for using stderr vs. stdout? http://cr.opensolaris.org/~johansen/pkg-mirror1/src/depot.py.wdiff.html ========== 66 +MIRROR_DEFAULT = False Can you add a comment to be consistent with the other default constants? 93 +Usage: /usr/lib/pkg.depotd [--readonly] [--rebuild] [--mirror] [-d repo_dir] 94 + [-p port] [-s threads] [-t socket_timeout] Since you are here... Is it really appropriate for us to place /usr/lib in the text here given our multi-platform nature? 98 + --mirror Content mirror mode; no metadata operations s/metadata operations/publishing and metadata operations disabled/ ? I know metadata technically includes publishing, but... 199 + if mirror: 200 + scfg.set_mirror() 201 + Should mirror be allowed in combination with readonly or does it matter? http://cr.opensolaris.org/~johansen/pkg-mirror1/src/modules/client/filelist.py.wdiff.html ========== 398 + def _pick_mirror(self, chosen_set = None): Drop spaces around '=' for PEP8 style. 410 + chosen_set.add(self.ds) Newline needed after this line. 416 + remove the file and raise a InvalidContentException.""" s/a/an/ http://cr.opensolaris.org/~johansen/pkg-mirror1/src/modules/client/image.py.wdiff.html ========== 311 + def select_mirror(self, auth = None, chosen_set = None): Drop spaces around '=' for PEP8. 313 + the mirrors. Pick the best one. Returns DepotStatus object.""" s/one. Returns/one and return a/ 452 + def add_mirror(self, auth_name, mirror): 453 + """Add the mirror URLs contained in mirror to 454 + auth_name's list of mirrors.""" Can you expand the comment to include the expected format of mirror since it is multiple URLs? Or was this supposed to be URL instead of URLs? 457 + auths[auth_name]["mirrors"].append(mirror) What about a try: except: to handle the auth_name not existing? 460 + def has_mirror(self, auth_name, url): 461 + """Returns true if url is in auth_name's list of mirrors.""" 462 + 463 + return url in self.cfg_cache.authorities[auth_name]["mirrors"] What about a try: except: to handle the auth_name not existing? 465 + def del_mirror(self, auth_name, mirror): Nitpick: Should this be delete instead of del to be consistent with delete_authority also in this file? 471 + if mirror in self.cfg_cache.authorities[auth_name]["mirrors"]: 472 + auths[auth_name]["mirrors"].remove(mirror) What about a try: except: to handle the auth_name not existing? 473 + self.cfg_cache.write("%s/cfg_cache" % self.imgdir) It seems like the configuration filename should be a class or instance attribute of self so that we don't have to keep hard-coding the path like this -- that or a wrapper method that we can call on self to save our configuration. http://cr.opensolaris.org/~johansen/pkg-mirror1/src/modules/client/imageconfig.py.wdiff.html ========== Nitpick: Why isn't it bad_tx and good_tx instead of errors and good_tx? 176 + self.flush_conten_cache = \ 177 + self.policies["flush-content-cache-on-success"] s/conten_cache/content_cache/ 220 + @staticmethod 221 + def read_list(str): Seem like this could go into pkg.misc. http://cr.opensolaris.org/~johansen/pkg-mirror1/src/modules/misc.py.wdiff.html ========== 326 + def __init__(self, args = None): Drop spaces around '=' for PEP8. 335 + str = "Action with path %s should have hash %s. Computed hash %s instead" % \ Should this end with '.' ? http://cr.opensolaris.org/~johansen/pkg-mirror1/src/modules/server/repository.py.wdiff.html ========== 187 + raise cherrypy.HTTPError(httplib.NOT_FOUND, 188 + "Operation not supported in current server mode.") Shouldn't this be 501 Not Implemented? 281 + fs = os.stat(opath) 282 + action.attrs["pkg.csize"] = str(fs[stat.ST_SIZE]) A comment above this explaining what we're doing, or an update to the docstring for the containing function would be nice. ---------- General Comments ---------- Can we open a bug for adding a test that actually ensures a client gets files from the mirror? Could possibly start two depots (one in normal mode, one in mirror mode) and then check the depot status object for the mirror to see if it actually got the transaction from there. Can we open a few bugs about the other topics that came up such as mirror synchronization, mirror authentication, etc.? I assume we'll pre-populate cfg_cache with a known set of mirrors for future releases? Cheers, -- Shawn Walker _______________________________________________ pkg-discuss mailing list [email protected] http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
