[tahoe-dev] [tahoe-lafs] #1106: review #1037 (SFTP)

[tahoe-lafs]

#1106: review #1037 (SFTP)
------------------------+---------------------------------------------------
     Reporter:  zooko   |       Owner:  zooko        
         Type:  defect  |      Status:  assigned     
     Priority:  major   |   Milestone:  1.7.1        
    Component:  code    |     Version:  1.7.0        
   Resolution:          |    Keywords:  review-needed
Launchpad Bug:          |  
------------------------+---------------------------------------------------

Comment (by zooko):

 Review notes (the review is not complete)

 David-Sarah: overall this is excellent work! It is readable and correct,
 with a few but not many parts that are un-idiomatic.

 Things that need to be responded to:
 * Hey look {{{sftpd.noisy}}} is {{{True}}}. Is that intentional?
 * Maybe document this with "assert" (and check it, too, when asserts are
 compiled in): {{{# invariant: self.download_size <= self.current_size}}}
 * Is it our job to unregister any extant producer before
 {{{self.producer=p in registerProducer()}}}?
 * !! Is that a busy eventually loop in _iterate() ? Bad!
 * add doc for self.async. I guess it means download is finished.
 * add doc for _convert_error()
 * data[offset:min(offset+length, len(data))] is the same as
 data[offset:offset+length]
 * Why do we need this catch-all-exceptions? Because err.value is not
 always an exception? This is noisy to the reader of source code.
 {{{
     try:
         if noisy: logmsg(traceback.format_exc(err.value), level=NOISY)
     except:  # pragma: no cover
         pass
 }}}
 * "The message argument to SFTPError must not reveal information that
 might compromise anonymity." Huh? Who's anonymity might be revealed to
 whom?
 * The following TODO can be removed based on testing in the 1.7.0
 development cycle (and feedback from 1.7.0 release) {{{TODO: check that
 clients are okay with this being a "?". (They should be because the
 longname is intended for human consumption.)}}}


 Ideas that don't necessarily need to be responded to:
 * class !OverwriteableFileConsumer could use unit tests
 * I think ".finish()" means the download is finished but local writes can
 proceed apace; docstring needed; Oh! I guess (based on the name) that it
 is defined by IFinishableConsumer. Maybe docstring that fact.
 * {{{# TODO: use download interface described in #993 when implemented.}}}
 I don't like "TODO" in comments in source code. They aren't usually
 noticed by the people who need to be reminded, and they often become stale
 (the thing no longer needs to be done, but nobody thinks to find the TODO
 and remove it.) I prefer issue tickets. This "TODO" should probably be
 deleted or moved to an issue ticket or maybe do the thing that it says to
 do. :-)
 * I find "eventually_callback(d)(expr)" harder to understand than
 "eventually(d.callback, expr)". (They do mean the same thing, right?) On
 the other hand I find self.async.addCallbacks(..., eventually_errback(d))
 to be mor easily read than any alternative.
 * idea: encourage users to experiment with hacking their SIZE_THRESHOLD,
 or perhaps even better do some more systematic measurements of how long it
 takes to download an entire file vs. to read parts out of a larger file in
 order to choose a more optimized SIZE_THRESHOLD.

-- 
Ticket URL: <http://tahoe-lafs.org/trac/tahoe-lafs/ticket/1106#comment:2>
tahoe-lafs <http://tahoe-lafs.org>
secure decentralized file storage grid