[tahoe-lafs-trac-stream] [tahoe-lafs] #1363: refactor storage_client.py, use IServer objects instead of rrefs

[tahoe-lafs]

#1363: refactor storage_client.py, use IServer objects instead of rrefs
------------------------+---------------------------
     Reporter:  warner  |      Owner:  zooko
         Type:  task    |     Status:  assigned
     Priority:  major   |  Milestone:  1.9.0
    Component:  code    |    Version:  1.8.2
   Resolution:          |   Keywords:  review-needed
Launchpad Bug:          |
------------------------+---------------------------

Comment (by zooko):

 attachment:1363-p3.dpatch reviewed. This is all really good stuff—I'm glad
 to see this sort of clean-up branch. I'm sorry it took me so long to
 review it. I intend to really elevate the priority of reviewing patches in
 my day to day life so that whenever Brian posts a new patch {{{review-
 needed}}}, I drop everything and review it right away.


 Patches that get +1 from me and I intend to commit them to trunk soon:

 * [http://tahoe-lafs.org/trac/tahoe-
 lafs/attachment/ticket/1363/1363-p3.dpatch#L548 test_immutable.Test:
 rewrite to use NoNetworkGrid, now takes 2.7s not 97s]
     By the way, on my Macbook Pro,
 {{{allmydata.test.test_immutable.Test}}} takes 8s, not 97s! After the
 patch "test_immutable.Test: rewrite to use !NoNetworkGrid" then it takes
 about 2s on my system. This isn't an issue with the patch, but it may
 indicate there is an issue with your system. Improving the speed of the
 tests from 8s to 2s is valuable even if your system could be changed to
 run the old tests in a mere 8s. You could look at the timings of the
 buildslaves for comparison, e.g.: [http://tahoe-
 lafs.org/buildbot/builders/FranXois%20lenny-
 armv5tel/builds/486/steps/test/logs/timings FranXois lenny-armv5tel],
 [http://tahoe-
 lafs.org/buildbot/builders/Brian%20ubuntu-i386%20linode/builds/252/steps/test/logs/timings
 Brian ubuntu-linode], [http://tahoe-
 lafs.org/buildbot/builders/Arthur%20lenny%20c7%2032bit/builds/739/steps/test/logs/timings
 Arthur lenny-c7-32bit], [http://tahoe-
 lafs.org/buildbot/builders/FreeStorm%20WinXP-x86%20py2.6/builds/543/steps/test/logs/timings
 FreeStorm WinXP-x86].
 * [http://tahoe-lafs.org/trac/tahoe-
 lafs/attachment/ticket/1363/1363-p3.dpatch#L842 remove now-unused
 ShareManglingMixin]; Would have been better included in the previous
 patch. I'll rerecord them together.
 * [http://tahoe-lafs.org/trac/tahoe-
 lafs/attachment/ticket/1363/1363-p3.dpatch#L40 apply zooko's advice:
 storage_client get_known_servers() returns a frozenset, caller sorts]
 * [http://tahoe-lafs.org/trac/tahoe-
 lafs/attachment/ticket/1363/1363-p3.dpatch#L1320
 DownloadStatus.add_known_share wants to be used by Finder, web.status];
 Wouldn't it be better to open a ticket than to put an {{{XXX}}} comment in
 the source code? Otherwise +1 on this patch.
 * [http://tahoe-lafs.org/trac/tahoe-
 lafs/attachment/ticket/1363/1363-p3.dpatch#L253 replace IServer.name()
 with get_name(), and get_longname()] and [http://tahoe-lafs.org/trac
 /tahoe-lafs/attachment/ticket/1363/1363-p3.dpatch#L76 upload.py: apply
 David-Sarah's advice rename (un)contacted(2) trackers to
 first_pass/second_pass/next_pass]; +1, but I rerecorded them to use
 {{{darcs replace}}} instead of hunk editing, because that avoided a lot of
 unnecessary merge conflicts with Kevan's #1382 branch. I'll attach my
 rerecorded patch to this ticket. I also took notes during the process of
 merging these with #1382, which notes I'll post to tahoe-dev later,
 explaining why {{{darcs replace}}} was so valuable in this case. Also
 [http://tahoe-lafs.org/trac/tahoe-
 lafs/attachment/ticket/1363/1363-p3.dpatch#L209 in set_shareholders()] the
 docstring is now wrong in describing the "upload_trackers" and
 "already_serverids". I'll change my rerecord of the patch to fix that.

 Patches with issues:
 * [http://tahoe-lafs.org/trac/tahoe-
 lafs/attachment/ticket/1363/1363-p3.dpatch#L1145 remove get_serverid from
 DownloadStatus.add_request_sent and customers] and [http://tahoe-
 lafs.org/trac/tahoe-lafs/attachment/ticket/1363/1363-p3.dpatch#L1013
 remove get_serverid from DownloadStatus.add_dyhb_sent and customers];
 These two had lots of merge conflicts with trunk. I rebased these patches
 onto the current head of trunk and will attach them to this ticket for
 Brian (if available) to review.
 * [http://tahoe-lafs.org/trac/tahoe-
 lafs/attachment/ticket/1363/1363-p3.dpatch#L1272 web/status.py: remove
 spurious whitespace, no code changes]; merge conflicts; an unimportant
 patch; I ran {{{M-x whitespace-cleanup}}} on this file and emacs made only
 one change (removing a trailing blank line at end of file).
 * [http://tahoe-lafs.org/trac/tahoe-
 lafs/attachment/ticket/1363/1363-p3.dpatch#L1334 remove nodeid from
 WriteBucketProxy classes and customers]; Shouldn't the {{{rref}}}
 parameter to {{{make_write_bucket_proxy()}}} be removed in favor of using
 the {{{.get_rref()}}} method of the {{{server}}} argument? Otherwise +1
 * [http://tahoe-lafs.org/trac/tahoe-
 lafs/attachment/ticket/1363/1363-p3.dpatch#L1515 remove get_serverid()
 from ReadBucketProxy and customers, including Checker] likewise, shouldn't
 the {{{ReadBucketProxy}}} stop accepting an {{{rref}}} argument now that
 it has a {{{server}}} argument to its constructor? Otherwise +1. If Brian
 (or someone) tells me that these two patches should go in as-is without
 removing the {{{rref}}} parameter, I'm okay with that.

-- 
Ticket URL: <http://tahoe-lafs.org/trac/tahoe-lafs/ticket/1363#comment:26>
tahoe-lafs <http://tahoe-lafs.org>
secure decentralized storage