[tahoe-lafs-trac-stream] [tahoe-lafs] #1382: immutable peer selection refactoring and enhancements

[tahoe-lafs]

#1382: immutable peer selection refactoring and enhancements
-------------------------+-------------------------------------------------
     Reporter:  kevan    |      Owner:  markberger
         Type:           |     Status:  new
  enhancement            |  Milestone:  1.11.0
     Priority:  major    |    Version:  1.8.2
    Component:  code-    |   Keywords:  review-needed servers-of-happiness
  peerselection          |  blocks-release
   Resolution:           |
Launchpad Bug:           |
-------------------------+-------------------------------------------------

Comment (by zooko):

 Mark:

 Here are some more review comments. These are from looking at
 [https://github.com/markberger/tahoe-
 lafs/blob/fa6f3cfb1e258e4427f35a7aada05c7ad2c9dd13/src/allmydata/immutable/upload.py#L340
 upload.py on the current version of your 1382-rewrite-2 branch].

 * This [https://github.com/markberger/tahoe-
 lafs/blob/fa6f3cfb1e258e4427f35a7aada05c7ad2c9dd13/src/allmydata/immutable/upload.py#L372
 comment on line 372] is totally vestigial, right? It is describing the old
 upload algorithm that you've now replaced.
 * [https://github.com/markberger/tahoe-
 lafs/blob/d9c7dfa4a0085884a6a3ba256e558e316fde096c/docs/architecture.rst
 #server-selection architecture.rst#server-selection] is describing the old
 algorithm. It should probably be replaced with a one-sentence description
 and a link to [https://github.com/markberger/tahoe-
 lafs/blob/603333f9bba4ef4c40f970f93baa255eeca008c8/docs/specifications
 /servers-of-happiness.rst#upload-strategy-of-happiness servers-of-
 happiness.rst#upload-strategy-of-happiness].
 * I'm thinking about the constant {{{2}}} at
 [https://github.com/markberger/tahoe-
 lafs/blob/fa6f3cfb1e258e4427f35a7aada05c7ad2c9dd13/src/allmydata/immutable/upload.py#L340
 line 340]. A reader might wonder why that is there, and why it isn't
 {{{3}}} or {{{2.71828}}} or something. We should add a comment next to it
 saying something like "If there are a large number of servers, we don't
 want to spend a lot of time and network traffic talking to all of them
 before deciding which ones to upload to. Here we limit our consideration
 to the first 2*N servers. That ought to be enough. See docs/specifications
 /servers-of-happiness.rst for details.". Then add to servers-of-
 happiness.rst a discussion about that constant 2.
 * Hm…in fact, now that I think about it, I'm not happy about this part of
 the behavior. Actually, I think I'll save that for a separate comment!
 * I find [https://github.com/markberger/tahoe-
 lafs/blob/fa6f3cfb1e258e4427f35a7aada05c7ad2c9dd13/src/allmydata/immutable/upload.py#L418
 self.tasks] confusing. The following things would probably make it easier
 for me to follow:
   - change the name to something more specific than "tasks". Is it…an
 "upload_plan"?
   - add a comment next to it saying what it is, possibly: "a map from
 shnum to a set of servers that the share should be uploaded to"
   - actually you can skip the comment if the renaming above makes it
 obvious to the reader that it is the return value from
 [https://github.com/markberger/tahoe-
 lafs/blob/fa6f3cfb1e258e4427f35a7aada05c7ad2c9dd13/src/allmydata/immutable/happiness_upload.py#L26
 Happiness_Upload.generate_mappings], especially if you rename
 {{{generate_mappings}}} to {{{calculate_upload_plan}}}. ☺
 * Relatedly, the word {{{servermap}}} is used in this file on
 [https://github.com/markberger/tahoe-
 lafs/blob/fa6f3cfb1e258e4427f35a7aada05c7ad2c9dd13/src/allmydata/immutable/upload.py#L54
 line 54] to denote a data structure which is shaped like {{{{serverid:
 set(shnum)} }}}, but [https://github.com/markberger/tahoe-
 lafs/blob/fa6f3cfb1e258e4427f35a7aada05c7ad2c9dd13/src/allmydata/immutable/upload.py#L482
 here] a "servermap" is a datastructure which is shaped like {{{{shnum:
 serverid} }}}, and then [https://github.com/markberger/tahoe-
 lafs/blob/fa6f3cfb1e258e4427f35a7aada05c7ad2c9dd13/src/allmydata/immutable/upload.py#L1025
 further down in the file] "servermap" is used to mean something which is
 shaped like {{{{shnum: set(serverids)} }}}. Confusing! (Even though
 "servermap" is a reasonable word for any of these three shapes.) Could we
 think of different local names for the latter two? Better not touch the
 first one since it is transmitted over the wire in the helper protocol,
 apparently.) Oh, here's a partial solution: the one on line 482 doesn't
 need to be named at all — just inline it instead of using a variable to
 hold it between line 482 and 483. ☺
 * Please rename {{{_isHappinessPossible}}} to {{{is_happiness_possible}}}
 and {{{Happiness_Upload}}} to {{{HappinessUpload}}}, to match
 [wiki:CodingStandards our conventions].

 Okay, I didn't really ''finish'' studying the entire upload.py (this
 version of it), but I'm stopping here for tonight. ☺

-- 
Ticket URL: <https://tahoe-lafs.org/trac/tahoe-lafs/ticket/1382#comment:55>
tahoe-lafs <https://tahoe-lafs.org>
secure decentralized storage