[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/ > > Thanks, > > -j > _______________________________________________ > pkg-discuss mailing list > [email protected] > http://mail.opensolaris.org/mailman/listinfo/pkg-discuss > I agree with Shawn that -m and -M aren't particularly intuitive. Maybe -am and -rm?
http://cr.opensolaris.org/~johansen/pkg-mirror1/src/client.py.sdiff.html line 1333: I'd say "removed mirror" instead of just "removed" http://cr.opensolaris.org/~johansen/pkg-mirror1/src/depot.py.sdiff.html It might be worth checking the flags set to make sure they're sane, for example a --mirror with --refresh-refresh index wouldn't make sense. (the cr for 2693 that's out now has a 3 of these) and maybe we should abstract them into a check_sanity function, but don't worry about that. Anyway, I think checking the matches of --mirror with the other flags would probably be helpful. http://cr.opensolaris.org/~johansen/pkg-mirror1/src/modules/client/filelist.py.sdiff.html line 226: I think in the future, we might want to treat these two errors differently, I'd be much more tolerant of a depot that timesout than one that actively gives me bad data. Definitely a future change, just thought I'd mention it. Though, on 234, we might want to distinguish between getting a time out from repeatedly getting bad data, both to the user and to our actions. Maybe if we get bad data, we just keep trying other mirrors until we have no more mirrors to try? Not sure that's the right the behavior either, but there's probably a happy medium somewhere. http://cr.opensolaris.org/~johansen/pkg-mirror1/src/modules/client/image.py.sdiff.html line 311: can you add a comment/more doc string about what the chosen_set argument is for? I think it's the set of mirrors you want to exclude from being selected, probably because they've been chosen previously, but that wasn't obvious till I delved into the code. http://cr.opensolaris.org/~johansen/pkg-mirror1/src/modules/server/repository.py.sdiff.html line 156: might we want to log (or debug log when that comes in) the unsupported actions here? It might simplify things when people are going, "why isn't my depot supporting search?" (Or perhaps log a list of the supported requests instead?) line 357: I'm confused why we're skipping a file if it's not there, rather than aborting or throwing an exception. I'm probably just not understanding how that'll be detected client side and what the recovery method will be. Eventually, I'd like to see some test suite support for testing mirroring. But that can wait (IMO) if your crunching to get this in soon or if the test suite can't support testing mirrors (though I'd think it would). That's all I found. The only critical things I think are the -m -M issue and understanding what repository.py line 357 is doing. Thanks, Brock _______________________________________________ pkg-discuss mailing list [email protected] http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
