On Fri, Dec 11, 2009 at 04:47:05PM -0800, Danek Duvall wrote: > (Feel free to reply back on-list; I'm just replying personally since you > did.)
Ack, sorry. I thought I replied to the list, but I guess not. > > On Fri, Dec 11, 2009 at 03:10:52PM -0800, Danek Duvall wrote: > > > [email protected] wrote: > > > > > > > http://cr.opensolaris.org/~johansen/webrev-13199/ > > > > > > Handful of nits ... > > > > > > transport/engine.py: > > > > > > - line 151: this comment talks about pretransfer time, but the code it > > > refers to (line 156) is about start transfer time. > > > > Ok. > > This still stands; perhaps I wasn't clear. The comment reads "[ ...] just > use pretransfer time", but then you set seconds to something based on a > variable called STARTTRANSFER_TIME, not PRETRANSFER_TIME, which is what > you'd expect when the comment says "pretransfer time". Should the comment > say "just use start transfer time" or should the code use PRETRANSFER_TIME? Sorry, I wrote starttransfer time and then cleaned up the sentence. I somehow managed to get another "pretransfer" back in there. It's removed for good now. > > > transport/stats.py > > > > > > - line 49: mode 0? > > > > Ack. That's actually a buffer size of 0, but all of that should be > > removed, since it was just there for debugging. Sorry about that. > > Cool. But you still set self.outf to None in the init method -- can that > be deleted, too? Yes, an omission on my part. Fixed. > > > - line 417: Why do you have a Cospeed_none, but not a Cocspeed_none? > > > > It's always 1 if origin_cspeed is zero. Look at 356-359. > > I think my point was simpler than that -- you created a constant, > Cospeed_none, rather than just setting ospeed to 100000 on (now) line 345. > Why not do the same for ocspeed, too? It looks imbalanced, and I don't see > the reason for it. Ok, I'll make this change to quality(). > Why did the __str__() method disappear in the new rev, anyway? It was just there for debugging. The whole thing is redundant once we're not logging instantaneous information about every term in the quality equation. > > > - line 419: why define a function just to set one variable, once? > > > > This is a Heaviside step function. It made sense to me to write this as > > a function. > > > > http://en.wikipedia.org/wiki/Heaviside_step_function > > Doesn't look like what you've defined, which has a plateau at some level > (not 1), and then drops back to 0 later. But if it's a variation, it might > be useful to put a comment saying why you've chosen to do it this way. It was initially 0 -> fixed value, but after some testing we decided to make that value decrease as the host got more use. I'm still not sure I understand this objection, though. There are functions within functions in many different places in our code. I thought it was easier to see that this was a function, and a term in the quality equation, when written the way that it is. -j _______________________________________________ pkg-discuss mailing list [email protected] http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
