[tahoe-lafs-trac-stream] [tahoe-lafs] #1382: immutable peer selection refactoring and enhancements
tahoe-lafs
trac at tahoe-lafs.org
Tue Sep 3 03:50:57 UTC 2013
#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: I started doing real code review while on a plane to Washington D.C.
today. Here are my notes. I apologize if they are confusing or incomplete.
I'm deciding I should send you these notes now instead of sleeping on it,
in order to not delay.
Overall, these patches are really great! You've done a fantastic job of
satisfying all the requirements, recording the patches into coherent
chunks, etc. I'm making a lot of requests of you in the following review
notes, and I hope it doesn't discourage you! I'm excited about getting
#1382 fixed in trunk ASAP!
regarding https://github.com/markberger/tahoe-
lafs/commit/739468e69676d30353814248f1fd7dd4b7b9d8f9:
* "servers- of" → "servers-of"
* "1. Query all servers for existing shares." — actually only 2*N servers?
* s/an arbitrary/a/g
* define (explain) "maximum matching graph" the first time it occurs
* link from servers-of-happiness.rst to upload-of-happiness.rst, or
actually better to combine the two documents together.
* link to Kevan's thesis from this doc
* Hey, I think a series of diagrams showing an example graph would help a
lot.
* In step 2, "a server s" → "a readonly server s"
* Shouldn't step 8 be broken into two steps?
* Shouldn't failures of renewals in step 8 be handled by removing the
server and starting over, just a failures of placements are in step 9?
* "placement is not generated for a subset of shares" ; Hey! This is Diego
"sickness" Righi's long-standing request (comment:12:ticket699).
* "guarantees that N shares will be placed" → "guarantees that at least H
shares will be placed"
regarding https://github.com/markberger/tahoe-
lafs/commit/ff1a77924b03d286477496c0e0a3ddfe45353c61:
* Move the explanation about what a "flow network" is and what a "maximum
spanning" is up to before the first occurrence of those terms.
* No wait, instead move the explanation about what a "flow network" is
into upload-of-happiness.rst and link to that document!
* Also, explain if the flow network in this case is going to just be an
integral flow of 0 or 1, in which case… maybe it doesn't matter that it is
a flow network?
* I'm confused about whether generate_mappings() is analyzing only the
existing allocations, as the docstring seems to indicate, or additionally
proposing new allocations, as the last comment within it indicates.
* Are "readonly_peers" and "peerids" the same type of thing? Maybe named
the former "readonly_peerids" then.
* "[key for key in servermap]" → "servermap.keys()"
* change:
{{{
self.servermap_shareids = set()
for key in servermap:
for share in servermap[key]:
self.servermap_shareids.add(share)
}}}
to:
{{{
self.servermap_shareids = set(servermap.values())
}}}
* No wait, actually remove {{{self.servermap_shareids}}} and
{{{self.servermap_peerids}}} entirely and use the appropriate expression
instead of those member variables where they are currently used.
* For _flow_network():
* "Given set of peerids and shareids" → "Given a set of peerids and a
set of shareids"
* Explain what the type of the return value is. It is a list of…
lists… of something? Explain the data structure — what shape it has. I
see that over in happinessutil.py it is mentioned "graph is a flow network
in adjacency-list form". Is this the same? Maybe just put that in the
docstring of all functions that take or return that type. However, I would
also like more explanation of what adjacency-list form is, perhaps in
happinessutil.py somewhere, and the other uses of it can say "As defined
in happinessutil.py" ?
* Write a unit test showing that _flow_network() returns the right
answer for a range of inputs.
* Okay, okay, hold on.
allmydata.immutable.happiness_upload.Happiness_Upload._flow_network is
almost identical to allmydata.util.happinessutil.flow_network_for. And,
the latter looks like it has more of the kind of documentation that I'm
asking for here. (But not yet the unit test that I asked for here.) Also
allmydata.immutable.happiness_upload.Happiness_Upload._servermap_flow_graph.
So: can we refactor these three functions into one function? And write
unit tests for it?
Okay, I'm stopping reviewing [ff1a77924b03d286477496c0e0a3ddfe45353c61]
for now because I don't know how much of it is duplicated or near-
duplicated code…
regarding https://github.com/markberger/tahoe-
lafs/commit/e7af5f7995bc661d19d5c903359813fa91880966 and
https://github.com/markberger/tahoe-
lafs/commit/5875c31d32ef50467c572736599066fffe8b73ac:
* In the docstring for get_tasks, "allocations" → "placements",
"allocation" → "placement", "reliably or correctly placed" → "well
allocated"
* Add to the confirm_share_allocation docstring "i.e. the share has been
successfully uploaded to the specified server" (if that is correct).
* add_peers isn't used, but an "add_peer" method that isn't included in
this interface is. (Perhaps this would have been detected by some kind of
interface-checking tool? That Daira might have written? I don't remember…)
* Likewise with "add_peer_with_share" and "add_peer_with_shares".
* mark_full_peer is used on each of the servers detected as full by
examining their announcements of themselves, but is not used elsewhere.
This makes me wonder what happens if a server is not full according to its
announcements, but it has become full since the creation of that
announcement and before this upload begins, or it becomes full during this
upload. Should such servers be marked with "mark_full_peer"? If so, then
we should write a unit test that is red with the current code and goes
green when we change the code to mark such servers as full.
* I would prefer if the tasks object returned from get_tasks in upload.py
were passed around as arguments instead of stored in a member variable.
Member variables can be updated or read from more places, and can persist
longer, so I have to look at a wider scope of code when wondering what
could be effected by or effecting it. Arguments have narrower scope, and
so are preferable except when you actually do want the state to be
reachable from all (or at least most) of the methods.
* I don't think is_healthy is used. There is a different "is_healthy"
defined in checker results that is used a lot, but as far as I could see
from grep none of the uses of is_healthy are the one from this interface.
So, good! Maybe it is unnecessary and can be removed.
* I don't think needs_recomputation is necessary or used.
* change:
{{{
def add_peer_with_shares(self, peerid, shnum):
if peerid in self.existing_shares.keys():
self.existing_shares[peerid].add(shnum)
else:
self.existing_shares[peerid] = set([shnum])
}}}
to:
{{{
def add_peer_with_share(self, peerid, shnum):
self.existing_shares.setdefault(peerid, set()).add(shnum)
}}}
* for mark_bad_peer, you can use the set ".discard()" method to simplify
the code
--
Ticket URL: <https://tahoe-lafs.org/trac/tahoe-lafs/ticket/1382#comment:41>
tahoe-lafs <https://tahoe-lafs.org>
secure decentralized storage
More information about the tahoe-lafs-trac-stream
mailing list