1382.markberger rewrite rebase.6 #416

meejah · 2017-04-25T05:17:57Z

This is a followup to #402

...could be useful to look at the diff from 'there to here': meejah/tahoe-lafs@74362a4...meejah:4d2b11f

warner · 2017-06-05T10:12:10Z

Except for the downloader issue, I'm ok with landing this now and figuring out the rest later. The weight of history is bearing down on this branch :). We should think about the timeout issue, and the how-many-servers-will-we-use issue, before a release, but I think we should land this first.

warner · 2017-06-05T09:58:48Z

src/allmydata/immutable/downloader/finder.py

@@ -63,6 +63,7 @@ def start_finding_servers(self):
        if not self._started:
            si = self.verifycap.storage_index
            servers = self._storage_broker.get_servers_for_psi(si)
+            servers.sort(key=lambda s: s.get_serverid())


We should remove this change. It modifies the downloader behavior, causing it to search for shares in the wrong order (get_servers_for_psi() returns the permuted list, but this .sort() call throws away that permutation), which will be a significant number-of-round-trip performance impact when there are a lot of servers. Not sure how this snuck in, maybe some unit test wanted to compare the ordering of servers on both uploads and downloads?

Hm, removing it causes test_hung_server.HungServerDownloadTest.test_5_overdue_immediate to fail consistently. I'll look into that.

I agree this should be removed; it was in the original mark berger branch and I don't know why/what it's supposed to accomplish...

warner · 2017-06-05T10:17:00Z

Oh, also, integration/test_servers_of_happiness.py is failing (on travis), I think it's a new test that got added in this branch?

meejah · 2017-06-05T17:18:51Z

I will look into the integration thing; I think I actually made a unit-test that does the same things anyway. And yes, it got added in this branch by me.

meejah · 2017-06-05T17:19:46Z

p.s. do you want me to at least squash all my commits into one? i.e. everything after "Fix test test_lost_servers" by @david415?

warner · 2017-06-05T17:25:32Z

Yeah, squashing is probably a good idea. The unrulyness of this branch is beyond hope, but at least we can avoid making it worse with each new round of fixes :).

This is Mark Berger's original commits, from ticket #1382

…ing_response Add comments 10 and 8 from the servers of happiness spec Fix bug in _filter_g3 for servers of happiness Remove usage of HappinessUpload class here we modifying the PeerSelector class. we make sure to correctly calculate the happiness value by ignoring keys who's value are None... Remove HappinessUpload and tests Replace helper servers_of_happiness we replace it's previous implementation with a new wrapper function that uses share_placement

unit-test for happiness calculation unused function put old servers_of_happiness() calculation back for now test for calculate_happiness remove some redundant functions

Fix happiness test var names Remove unused imports Get rid of trailing whitespace

refactor hypothesis to be 'pytest style' and add another one get rid of 'shares->set(1 thing)' in generate_mappings return Add a unittest hypothesis came up with fix tests since we return peers, not sets-of-1-peer add more debug add a unit-test that's like test_problem_layout_ticket_1128 fix bug add a note fix utest unit-test for bigger numbers re-insert markberger code for testing results of pairing with david

Comment out all debug print statements Add hypothesis tests for the old servers of happiness implementation Attempt to speed up meejah's servers of happiness WIP Fix test_calc_happy WIP

correctly calculate happiness guard with except fix tests, and happiness calculation remove debug fix placements to None happiness calc shouldn't have to filter None WIP fixing some tests etc

Remove old hypothesis tests Fix allmydata.test.cli.test_cli.Errors.test_get this was broken due to differing share placements whereas we need to allow this. Fix test_5_overdue_immutable This change makes the test not depend on the value of PYTHONHASHSEED. Revert "Fix test_5_overdue_immutable" This reverts commit 5f3696d. fix test to actually hang the first 5 *servers* sort keys for stable output use file-context-managers remove probably-unneeded assert (that fails sometimes) another non-deterministic test?

codecov-io · 2017-06-05T22:27:01Z

Codecov Report

Merging #416 into master will decrease coverage by 0.03%.
The diff coverage is 96.8%.

@@            Coverage Diff             @@
##           master     #416      +/-   ##
==========================================
- Coverage   88.02%   87.98%   -0.04%     
==========================================
  Files         146      147       +1     
  Lines       27578    27833     +255     
  Branches     3921     3977      +56     
==========================================
+ Hits        24275    24489     +214     
- Misses       2599     2627      +28     
- Partials      704      717      +13

Impacted Files	Coverage Δ
src/allmydata/control.py	`68.44% <ø> (ø)`	⬆️
src/allmydata/nodemaker.py	`100% <ø> (ø)`	⬆️
src/allmydata/immutable/repairer.py	`100% <ø> (ø)`	⬆️
src/allmydata/dirnode.py	`98.26% <ø> (ø)`	⬆️
src/allmydata/immutable/checker.py	`87.94% <ø> (ø)`	⬆️
src/allmydata/immutable/downloader/share.py	`92.82% <ø> (-4.49%)`	⬇️
src/allmydata/util/happinessutil.py	`100% <100%> (ø)`	⬆️
src/allmydata/client.py	`86.95% <100%> (ø)`	⬆️
src/allmydata/interfaces.py	`98.84% <100%> (+0.01%)`	⬆️
src/allmydata/util/deferredutil.py	`70.73% <100%> (+4.69%)`	⬆️
... and 11 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 42d8a79...05f48c3. Read the comment docs.

Squashed all commits that were meejah's between 30d68fb and 33c268e On the branch meejah:1382.markberger-rewrite-rebase.6 as per review

warner · 2017-06-05T23:03:58Z

Ok, looks good! Landing momentarily.

meejah mentioned this pull request Apr 25, 2017

1382.markberger rewrite rebase.5 #402

Closed

meejah force-pushed the 1382.markberger-rewrite-rebase.6 branch from 4d2b11f to e5d6ff4 Compare April 25, 2017 22:12

warner requested changes Jun 5, 2017

View reviewed changes

meejah force-pushed the 1382.markberger-rewrite-rebase.6 branch 2 times, most recently from f086d8d to be73cd7 Compare June 5, 2017 21:57

warner approved these changes Jun 5, 2017

View reviewed changes

markberger and others added 10 commits June 5, 2017 16:26

Implements 'Servers of Happiness' algorithm for upload

17cff7a

This is Mark Berger's original commits, from ticket #1382

updates from summit

211dc60

WIP: refactoring (squahshed all meejah's commits)

adb9a98

fix happiness calculation

ef17ef2

unit-test for happiness calculation unused function put old servers_of_happiness() calculation back for now test for calculate_happiness remove some redundant functions

Add servers of happiness hypothesis testing

e68b331

Fix happiness test var names Remove unused imports Get rid of trailing whitespace

Make a correction to a hypothesis test comment

a611673

Comment out all debug print statements Add hypothesis tests for the old servers of happiness implementation Attempt to speed up meejah's servers of happiness WIP Fix test_calc_happy WIP

distribute only to read/write peers

56f6dbd

correctly calculate happiness guard with except fix tests, and happiness calculation remove debug fix placements to None happiness calc shouldn't have to filter None WIP fixing some tests etc

meejah force-pushed the 1382.markberger-rewrite-rebase.6 branch from be73cd7 to 23d27c5 Compare June 5, 2017 22:26

Various cleanups, fixes and improvements

05f48c3

Squashed all commits that were meejah's between 30d68fb and 33c268e On the branch meejah:1382.markberger-rewrite-rebase.6 as per review

meejah force-pushed the 1382.markberger-rewrite-rebase.6 branch from 23d27c5 to 05f48c3 Compare June 5, 2017 22:31

warner merged commit 05f48c3 into tahoe-lafs:master Jun 5, 2017

This was referenced Jan 9, 2018

1382 rewrite merged.1 #365

Closed

Immutable peer selection refactoring and enhancements #60

Closed

meejah deleted the 1382.markberger-rewrite-rebase.6 branch March 20, 2018 22:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

1382.markberger rewrite rebase.6 #416

1382.markberger rewrite rebase.6 #416

meejah commented Apr 25, 2017 •

edited

Loading

warner commented Jun 5, 2017

warner Jun 5, 2017

warner Jun 5, 2017

meejah Jun 5, 2017

warner commented Jun 5, 2017 •

edited

Loading

meejah commented Jun 5, 2017

meejah commented Jun 5, 2017

warner commented Jun 5, 2017

codecov-io commented Jun 5, 2017 •

edited

Loading

warner commented Jun 5, 2017

1382.markberger rewrite rebase.6 #416

1382.markberger rewrite rebase.6 #416

Conversation

meejah commented Apr 25, 2017 • edited Loading

warner commented Jun 5, 2017

warner Jun 5, 2017

Choose a reason for hiding this comment

warner Jun 5, 2017

Choose a reason for hiding this comment

meejah Jun 5, 2017

Choose a reason for hiding this comment

warner commented Jun 5, 2017 • edited Loading

meejah commented Jun 5, 2017

meejah commented Jun 5, 2017

warner commented Jun 5, 2017

codecov-io commented Jun 5, 2017 • edited Loading

Codecov Report

warner commented Jun 5, 2017

meejah commented Apr 25, 2017 •

edited

Loading

warner commented Jun 5, 2017 •

edited

Loading

codecov-io commented Jun 5, 2017 •

edited

Loading