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

Fallback to ActiveRecord::Base.connection #96

Closed
neilmiddleton opened this issue Aug 22, 2012 · 20 comments
Closed

Fallback to ActiveRecord::Base.connection #96

neilmiddleton opened this issue Aug 22, 2012 · 20 comments

Comments

@neilmiddleton
Copy link

I have an instance where I cannot set the database_url env vars (TDDium insists on using sockets). Would it be feasible for QC to fallback to use the ActiveRecord::Base.connection if there is one if the db_url won't resolve?

The only issue I can see at the moment is that the execute method will need changing to suit the different APIs

@ryandotsmith
Copy link
Contributor

This is a good issue. I have been thinking about how we could pass connections to QC. Is there an easy way to get an instance of PGconn from ActiveRecord?

@neilmiddleton
Copy link
Author

It seems that ActiveRecord::Base.connection.raw_connection might get us where we need to go. It seems to return a PG::Connection which is probably a good start.

@malclocke
Copy link
Contributor

The following seems to work in my Rails app in config/initializers/queue_classic.rb:

require 'queue_classic'

module QC
  module Conn
    def self.connection=(connection)
      unless connection.instance_of? PG::Connection
        raise(
          ArgumentError,
          "connection must be an instance of PG::Connection, but was #{connection.class}"
        )
      end
      @connection = connection
    end
  end
end

QC::Conn.connection = ActiveRecord::Base.connection.raw_connection

Am happy to send a pull request based on this, let me know.

@malclocke
Copy link
Contributor

Realised that it's easier to just send a PR, rather than ask if I should send a PR :). Feel free to reject if it's the wrong approach.

@neilmiddleton
Copy link
Author

Not sure which approach works better here now having see @malclocke's version - however, knowing how hard it was to get the AR connection in the right way (in terms of figuring it out) I feel that falling back to a predicted connection location might be better.

ryandotsmith pushed a commit that referenced this issue Nov 22, 2012
…ction

Allow setting of QC::Conn.connection (closes #96)
@felixbuenemann
Copy link

I don't think this is the right approach. I have the same problem, but for me it doesn't work because if I assign the connection in an initializer it might already be teared down/not pointing to the current connecton, when you try to access it.

I think a better approach would be to be able to specify a block that is yielded inside QC::Conn.connect to aquire the connection.

For now I'm solving this problem by overriding connect from an initializer:

module QC
  module Conn
    def connect
      conn = ActiveRecord::Base.connection.raw_connection
      if conn.status != PGconn::CONNECTION_OK
        log(:level => :error, :message => conn.error)
      end
      conn
    end
  end
end

@ryandotsmith
Copy link
Contributor

@fbuenemann but with your approach, queue_classic must know about ActiveRecord. Can you not delay the handoff of the active_record connection to queue_classic until you are sure that you have the right connection?

@felixbuenemann
Copy link

Well, my idea was to specify a block that is yielded inside QC, so the content of this block don't matter, as long a it yields a proper connection object.

Setting the connection object at a later point instead of in a initializer would probably work as well.

One thing I don't like about both cases, is that in case of an exception QC tears down the connection it didn't establish itself, which might be bad. For now I patched the disconnect method so it just resets @connection to nil.

@malclocke
Copy link
Contributor

@felixbuenemann I've just hit the problem you are describing above where we have a reconnect happening after fork in our production set-up.

I think the idea of setting a connection proc is a good one, e.g. something like:

QC.connection_proc = proc { ActiveRecord::Base.connection.raw_connection }

... and that proc will be used by QC::Conn.connect if provided, otherwise the default connection code is called.

Would be worth integrating this with #134 too I think.

I don't see any other way around this with our current production setup (unicorn), other than using DATABASE_URL.

@felixbuenemann
Copy link

I think there's a lot of duplication around connection handling already. There's connect and connection= and we would add yet another connection_proc=. I think a cleaner approach would be to have connect always relying on a proc to establish its connection and the default implementation would parse the database URI for building a connection. Then we could use connection= to pass in a proc that returns a connection. For backwards compatibility connection= could test wheter its param is a connection or a proc.

We would still need to tackle the disconnection problem though, because QC shouldn't be allowed to teardown a passed in connection. I'm not sure what's the best way to handle that.

@joevandyk
Copy link
Contributor

FWIW, I use sequel and activerecord and queue_classic in the same application. I would love to be able to share the same pg connection for everything.

I'd gladly pay someone to make this work! :)

@senny
Copy link
Contributor

senny commented Jun 9, 2013

This is the only thing missing for #134. I've had a lot of stuff to do lately but I hope to get it done in the upcoming days.

@joallard
Copy link

Seeing #134 was merged without this, maybe we should reopen the issue? @senny

@senny
Copy link
Contributor

senny commented Aug 17, 2013

@joallard even though I merged #134 without it it's still on my todo list. I'm working on a connection pool, which is based on AR so that you should have zero-config out of the box when running within a Rails app.

@joallard
Copy link

Ah okay, good to know! Thanks

@ericboehs
Copy link

I just ran into PG::Error: connection is closed in my integration tests. I fixed it with @felixbuenemann's patch.

Establishing my own connection didn't seem to help (gave a different error), although setting QC_DATABASE_URL did. I think @felixbuenemann's solution is better. @senny, I look forward to the ability to pass block to QC::Conn.

@senny
Copy link
Contributor

senny commented Sep 10, 2013

@ericboehs using the AR connection is not supported yet. QC currently does not establish a new connection once it gets closed so you will eventually run into exactly the error you are describing. There are some internal changes happening and using the AR-connection by default in a Rails application is still on my todo list.

I recommend setting QC_DATABASE_URL in the meantime.

@ericboehs
Copy link

Thanks for the recommendation. I wrapped my monkey patch with if Rails.env.development? || Rails.env.test?. Setting QC_DATABASE_URL caused rake db:drop to stop working. Not sure why but I'll just wait for your connection pooling stuff.

My app seems to be working in production as I'm using Heroku which sets DATABASE_URL by default.

@joallard
Copy link

My workaround was to simply use the rails-database-url gem

@ericboehs
Copy link

My workaround was to simply use the rails-database-url gem

I saw that. Oddly it caused my integration tests to indefinitely stall. ¯_(ツ)_/¯

A team member suggested we just do

ENV['DATABASE_URL'] ||= "postgresql://localhost/OUR_APP_NAME_#{Rails.env}"

Which works fine since DATABASE_URL is always set in our production envs and our dev/test envs use localhost with the current user and no password (we set pg to allow logins on loopback only).

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

No branches or pull requests

8 participants