Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Ticket #573: A server selection framework. #140

Closed
wants to merge 1 commit into from

Conversation

@linshu
Copy link

linshu commented Feb 5, 2015

The 3rd party server selection function can be passed in as
[client][server_selection_hook] in the tahoe.cfg file.
The format is python module syntax, like "module_a.submodule_b.function_c".
A default hook function "sort_servers" is implemented in allmydata.storage_client
to catch no 3rd party hook functoin configured case.


This change is Reviewable

@daira

This comment has been minimized.

Copy link
Member

daira commented Feb 7, 2015

Please fix the test failures on this branch, then we'll review it. (If you need any help, don't hesitate to ask on Freenode #tahoe-lafs or the tahoe-dev list.)

@linshu linshu force-pushed the linshu:573-server-selection branch 3 times, most recently from 7c04dd1 to 7be447c Feb 7, 2015
@linshu

This comment has been minimized.

Copy link
Author

linshu commented Feb 7, 2015

Fixed the build. Please review it. Thanks.

@linshu

This comment has been minimized.

Copy link
Author

linshu commented Feb 7, 2015

Daira,

I fixed the build. Please review it. :-)

Thanks a lot!
-Shu

On Fri, Feb 6, 2015 at 5:02 PM, Daira Hopwood notifications@github.com
wrote:

Please fix the test failures on this branch, then we'll review it. (If you
need any help, don't hesitate to ask on Freenode #tahoe-lafs or the
tahoe-dev list.)


Reply to this email directly or view it on GitHub
#140 (comment).

@daira daira added the review-needed label Feb 9, 2015
@daira daira self-assigned this Feb 9, 2015
@linshu linshu force-pushed the linshu:573-server-selection branch from 7be447c to a3682b2 May 7, 2015
Shu Lin
The 3rd party server selection function can be passed in as
[client][server_selection_hook] in the tahoe.cfg file.
The format is python module syntax, like "module_a.submodule_b.function_c".
A default hook function "sort_servers" is implemented in allmydata.storage_client
to catch no 3rd party hook functoin configured case.
@linshu linshu force-pushed the linshu:573-server-selection branch from a3682b2 to 677fbd7 May 7, 2015
@linshu

This comment has been minimized.

Copy link
Author

linshu commented May 8, 2015

Hi Daira,

I have fixed all you suggested. Please review it.

Thanks.

@zooko

This comment has been minimized.

Copy link
Member

zooko commented May 8, 2015

@daira

This comment has been minimized.

Copy link
Member

daira commented May 16, 2015

v1.10.1 is feature-frozen, but I will definitely review it for v1.11 after the v1.10.1 release.

:param key: Specifies a function of one argument that is used to extract a comparison key from Tahoe server list
element. The 3rd party can use this as a default comparison key if they don't want to provide their own
comparison algorithm but still want to keep using the server selection framework.
:return: A sorted server list

This comment has been minimized.

Copy link
@meejah

meejah Aug 14, 2016

Contributor

This seems like a complicated API to export to users -- what they're supposed to do with key seems unclear (e.g. what if it's not specified? can it be ignored if it is specified?). I think it would be better if the "sort_servers" function takes only a single argument (the servers list). Or, maybe the second argument should be peer_selection_index?

Then for this default implementation, you could move the permuted inner-method from get_servers_for_psi into this function and thus still call sorted in the same fashion (with key=permuted).

This comment has been minimized.

Copy link
@daira

daira Oct 11, 2016

Member

We discussed this in a Tahoe Nuts and Bolts meeting, and decided that this should be passed the peer_selection_index. If we don't pass that, then the hook does not have enough information to make changes to how the _permuted algorithm below works.

m = getattr(m, module_lvl_name)
return m(self.get_connected_servers(), key=_permuted)
except:
# Explicitly catch the wrong configuration in tahoe.cfg or 3rd party module is not installed case.

This comment has been minimized.

Copy link
@meejah

meejah Aug 14, 2016

Contributor

A bare "except" is bad. The most-general should be except Exception: but ideally something more-specific (i.e. catch a more-specific Exception subclass). Also, I would guard the "import stuff" separately from the "invoke the hook" part. Besides all that, though, this particular try/except doesn't actually do anything and can be removed.

@meejah meejah removed the review-needed label Sep 4, 2018
@exarkun

This comment has been minimized.

Copy link
Member

exarkun commented Jan 22, 2020

Thanks to everyone for their efforts so far. This looks like a good start. The review feedback hasn't been addressed yet and the feature overall needs test coverage and probably additional documentation.

Please feel free to re-open this PR if there's still interest in contributing here.

@exarkun exarkun closed this Jan 22, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.