allow all options of HTTPPool to be specified by users #33
Conversation
|
In a recent discussion about the replica number, @nf suggested a configuration struct that will be passed to the http pool. See #29 (comment) |
|
Right, it's indeed a cleaner approach, thanks for the pointer. I have refactored this change to encapsulate all options to HTTPPool in the type HTTPPoolOptions. |
|
looks good IMO |
|
Hi, @nf do you think this change makes sense in light of the suggestion you made in #29 (comment) ? With a couple of other changes, we are deviating further and further away from this main repo, which is not good. It'd be great if you could review this change to help us be closer to this main repo. Sorry to be possibly disturbing you and thanks in advance. |
|
|
||
| // ServeMux specifies the request multiplexer to be registered | ||
| // If blank, it defaults to http.DefaultServeMux. | ||
| ServeMux *http.ServeMux |
There was a problem hiding this comment.
This seems redundant, we should just return the pool and allow the user to register it where they want.
|
Right, registering an HTTPPool with a http.ServeMux should not be the responsibility of NewHTTPPoolOpts. I have refactored the code to make this the case. Thanks for seeing this. |
| // It registers itself as a PeerPicker and as an HTTP handler with the | ||
| // http.DefaultServeMux. | ||
| // NewHTTPPool initializes an HTTP pool of peers, and registers itself as a PeerPicker. | ||
| // For backwards compatibility, it also registers itself as an http.Handler with http.DefaultServeMux. |
|
I had a hard time describing the need for users to register the pool with a http.ServeMux. Fortunately, now it sounds clearer and more native. Thanks! |
|
I'd also like you to squash these commits into one, if you please. |
|
Sure, of course. However, as I actually haven't squashed commits before, I wonder are you able to confirm this is correct:
I am aware that this alters the history on the server, and thus am not sure is this correct? |
|
That should work, but instead of to return to the first commit in the sequence, with all your changes unstaged. Then |
|
Thanks for the instructions, they work nicely. The commits are now cleanly squashed into one. |
|
Could you please sign the Google CLA? Thanks. |
|
Yes, sure, I have signed it and submitted the CLA. On Mon, Oct 20, 2014 at 3:29 PM, Andrew Gerrand [email protected]
|
allow all options of HTTPPool to be specified by users
|
Thanks! |
|
No problem, thanks for all the great comments, too. |
The default constructor
NewHTTPPoolis making too many assumptions about how people would use the library. For example, for security purpose I'd want groupcache to listen on a different port by calling Handle on a different ServeMux other than thehttp.DefaultServeMux. Others have also requested to be able to specify the basePath perhaps to shorter paths for performance reasons #22 ?