> I agree with Shawn that -m and -M aren't particularly intuitive. Maybe > -am and -rm?
I'm not thrilled with this approach either. You might want to take a look at my response to Shawn. Please pester Stephen if you think this is really abominable. > http://cr.opensolaris.org/~johansen/pkg-mirror1/src/client.py.sdiff.html > line 1333: I'd say "removed mirror" instead of just "removed" Ok. > 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. Would you provide a little more detail about the combinations that you don't think are sane? These flags have appeared as I've merged with different incoming stuff, and nobody has a sanity check yet. I'd be interested to know what combinations you think don't make sense. > 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. It sounds like you may be getting confused about the exceptions. That's probably my fault. The verify code throws an InvalidContentException that the filelist code currently doesn't catch. I was under the impression that a corrupt file was a showstopper, but maybe not. A TransferContentException gets thrown when we get a read error off of the tar stream. This happens when a depot can't send us any of the files we asked for, so they instead send us an empty tar stream. We catch the read error, raise this exception, and then try another mirror. > 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. That's correct. I'll see what I can do about a comment here. > 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?) This seems like it would be a potential for a DOS, resource exhaustion, or at a minimum a logfile full of unhelpful errors. > 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. It's not safe to assume that every host will have every piece of content. If a client asks for something that the server doesn't have, the server should sent it what it does have, but let it get the rest from somewhere else. > 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). I agree that we need to figure out how to do this. Thanks for reviewing this. -j _______________________________________________ pkg-discuss mailing list [email protected] http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
