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

Change to use a single socket, to avoid fd sockets leaking… #7

Merged
merged 2 commits into from
Feb 19, 2014

Conversation

JustinAiken
Copy link
Owner

Because | grep "UDP" | wc -l should never show numbers in the 1000's, or you're going to have a bad time...

@bklang
Copy link
Contributor

bklang commented Feb 15, 2014

Something's not right, I'm getting

/srv/apps/ringplus_engine/shared/bundle/ruby/1.9.1/gems/statsd-ruby-1.2.1/lib/statsd.rb:116:in `initialize': wrong number of arguments (3 for 2) (ArgumentError)
    from /srv/apps/ringplus_engine/shared/bundle/ruby/1.9.1/bundler/gems/adhearsion-stats-f2c217ba9f8f/lib/adhearsion-stats/plugin.rb:20:in `new'
    from /srv/apps/ringplus_engine/shared/bundle/ruby/1.9.1/bundler/gems/adhearsion-stats-f2c217ba9f8f/lib/adhearsion-stats/plugin.rb:20:in `block in <class:Plugin>'
    from /srv/apps/ringplus_engine/shared/bundle/ruby/1.9.1/gems/adhearsion-2.5.0/lib/adhearsion/plugin/initializer.rb:26:in `instance_exec'
    from /srv/apps/ringplus_engine/shared/bundle/ruby/1.9.1/gems/adhearsion-2.5.0/lib/adhearsion/plugin/initializer.rb:26:in `run'
    from /srv/apps/ringplus_engine/shared/bundle/ruby/1.9.1/gems/adhearsion-2.5.0/lib/adhearsion/plugin.rb:168:in `block in init_plugins'
    from /srv/apps/ringplus_engine/shared/bundle/ruby/1.9.1/gems/adhearsion-2.5.0/lib/adhearsion/plugin.rb:167:in `each'
    from /srv/apps/ringplus_engine/shared/bundle/ruby/1.9.1/gems/adhearsion-2.5.0/lib/adhearsion/plugin.rb:167:in `init_plugins'
    from /srv/apps/ringplus_engine/shared/bundle/ruby/1.9.1/gems/adhearsion-2.5.0/lib/adhearsion/initializer.rb:241:in `init_plugins'
    from /srv/apps/ringplus_engine/shared/bundle/ruby/1.9.1/gems/adhearsion-2.5.0/lib/adhearsion/initializer.rb:63:in `block in start'
    from /srv/apps/ringplus_engine/shared/bundle/ruby/1.9.1/gems/adhearsion-2.5.0/lib/adhearsion/initializer.rb:41:in `catch'
    from /srv/apps/ringplus_engine/shared/bundle/ruby/1.9.1/gems/adhearsion-2.5.0/lib/adhearsion/initializer.rb:41:in `start'
    from /srv/apps/ringplus_engine/shared/bundle/ruby/1.9.1/gems/adhearsion-2.5.0/lib/adhearsion/initializer.rb:12:in `start'
    from /srv/apps/ringplus_engine/shared/bundle/ruby/1.9.1/gems/adhearsion-2.5.0/lib/adhearsion/cli_commands/ahn_command.rb:104:in `start_app'
    from /srv/apps/ringplus_engine/shared/bundle/ruby/1.9.1/gems/adhearsion-2.5.0/lib/adhearsion/cli_commands/ahn_command.rb:49:in `daemon'
    from /srv/apps/ringplus_engine/shared/bundle/ruby/1.9.1/gems/thor-0.18.1/lib/thor/command.rb:27:in `run'
    from /srv/apps/ringplus_engine/shared/bundle/ruby/1.9.1/gems/thor-0.18.1/lib/thor/invocation.rb:120:in `invoke_command'
    from /srv/apps/ringplus_engine/shared/bundle/ruby/1.9.1/gems/thor-0.18.1/lib/thor.rb:363:in `dispatch'
    from /srv/apps/ringplus_engine/shared/bundle/ruby/1.9.1/gems/thor-0.18.1/lib/thor/base.rb:439:in `start'
    from script/ahn:9:in `<main>'

@bklang
Copy link
Contributor

bklang commented Feb 15, 2014

Ok, figured out that I need to use your branch of statsd-ruby. Before we push this as a gem we'll have to address that.

@JustinAiken
Copy link
Owner Author

Aah right, forgot about that.. Wish bundler handled gems from git repos somehow...

I see a few ways to work around this issue:

  1. Just add a note to the readme that you need a fork of statsd-ruby in your app's Gemfile
  2. Release my fork of statsd as it's own gem (statsd-socket-ruby), require that
  3. Just embedded the stasd library directly into this gem...

Any preference or alternative ideas?

@bklang
Copy link
Contributor

bklang commented Feb 18, 2014

As much as I hate forks, it appears that upstream isn't likely to merge your changes.

If the amount of functionality provided is as small as I think it is, I'm inclined to go with Option #3.

@benlangfeld
Copy link

I'm not a fan of option 3 due to the duplication and maintenance overhead it creates. Option 1 is not an acceptable solution because it's unnecessarily complicated for the end user. Option 2 would be an acceptable solution, but an upstream merge would be preferable.

Have you been able to contact @raggi to push for your PR to be merged/released? It looks like there's some pending changes unreleased upstream for IPv6 support, and your changes would go nicely in a 1.3 release ;) I think if @raggi can be cajoled into getting that situation resolved, that would be the best for everyone, otherwise go ahead and release a gem from your fork.

@JustinAiken
Copy link
Owner Author

For now I've just forked the gem... if my PR ever gets merged, we can update the dependency back to the mainline...

JustinAiken pushed a commit that referenced this pull request Feb 19, 2014
Change to use a single socket, to avoid fd sockets leaking…
@JustinAiken JustinAiken merged commit 44e8568 into develop Feb 19, 2014
@JustinAiken JustinAiken deleted the feature/single_socket branch February 19, 2014 23:29
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.

3 participants