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

[DEVTOOLS-91] Adding connection limiter #116

Merged
merged 1 commit into from
Apr 28, 2015

Conversation

mmerrell
Copy link

Adding entries to .gitignore to aid in Mac development

[DEVTOOLS-91] Adding connection limiter
[DEVTOOLS-91] Updating README and config-example.json

Adding entries to .gitignore

[DEVTOOLS-91] Merging from master

Conflicts:
	.gitignore

[DEVTOOLS-91] Adding connection limiter

[DEVTOOLS-91] Removing log statements

[DEVTOOLS-91] Removing some cruft

[DEVTOOLS-91] Implementing review comments

[DEVTOOLS-91] A couple more little things

[DEVTOOLS-91] Updating README and config-example.json
@mmerrell
Copy link
Author

This PR should address issue-115

@kellegous
Copy link
Member

Also #113. Thanks so much for fixing this, I skimmed the change and it looks good. Token bucket in a channel was exactly what I planned to do. I'm a little behind in integrating PR's but I hope to get caught up very soon.

@@ -199,7 +199,7 @@ func init() {
// NOTE: The keys in the searcher map will be normalized to lower case, but not such
// transformation will be done on the error map to make it easier to match those errors
// back to the original repo name.
func MakeAll(cfg *config.Config) (map[string]*Searcher, map[string]error, error) {
func MakeAll(cfg *config.Config, connectionLimiter) (map[string]*Searcher, map[string]error, error) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Finally got around to merge this tonight, but I'm finding it doesn't build.

kellegous added a commit that referenced this pull request Apr 28, 2015
@kellegous
Copy link
Member

@mmerrell Can you take a look at #118 and make sure it still solves the problem you are having. I changed a couple of things:

  1. The concurrency limit is about more than just the outgoing connections. We need a limiter on indexers in general. So I have renamed the config to max-concurrent-indexers.
  2. I move the creation of the token bucket out of main and put it into the searcher. I also created a type to wrap the channel so we can have Acquire & Release semantics. I also moved a good chunk of the reindexing loop into its own function to make this a bit cleaner.

@kellegous kellegous merged commit c7c3af4 into hound-search:master Apr 28, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants