cfis / activejob-locking

MIT License
30 stars 15 forks source link

Redis port and password aren't supported #3

Open bkroeker opened 5 years ago

bkroeker commented 5 years ago

Hi there. I'm using the suo adapter on my project and I had to hack activejob-locking to pass through a port and password for the redis server to the underlying suo gem. I'd submit a pull request, but the activejob-locking gem's current methodology for distributed servers is kind of counter-intuitive for the redis case. The suo adapter, for example, just uses { host: options.hosts.first } and drops any other hosts in the array. Would I ideally pass in an array of ports and passwords which would each get truncated to the first element?

In any case, supporting ports and passwords, or entire connection strings, seems like a pretty basic feature, which I would like to see supported at some point.

Cheers. :)

cfis commented 5 years ago

ActiveJob::Locking.options.hosts = ['localhost']

In the suo adapter, the code is:

class SuoRedis < Base
    def create_lock_manager
      mapped_options = {connection: {host: self.options.hosts.first},
                        stale_lock_expiration: self.options.lock_time,
                        acquisition_timeout: self.options.lock_acquire_time}

      Suo::Client::Redis.new(self.key, mapped_options)
    end`

Notice it takes the first host specified in options. So configure that value based on the suo documentation.

bkroeker commented 5 years ago

@cfis Thanks, but I don't think you read my post. I'm trying to specify ports and passwords, and/or entire connection strings.

cfis commented 5 years ago

Depending on the adapter, I think you can use the host string to specify the port and/or password.

bkroeker commented 5 years ago

In this case, to set the full connection string, you'd need to drop the host hash. So:

mapped_options = { connection: "127.0.0.1:11211", ... }

Basically, with the current implementation, it is impossible to specify ports or passwords using the Suo adapter. The create_lock_manager method you've printed out above is the problem -- it needs to be opened up to be more flexible. I could submit a pull request to do so, but I wasn't sure what strategy to employ in opening that method up, since doing so would make it inconsistent from the other adapters.

Maybe it needs to do something like mapped_options.merge!(self.options.adapter_options) to overwrite the limitations you've hardcoded into the method, kind of like you're doing in the RedisSemaphore adapter?

IMO, you should get rid of options.hosts completely and just have options.adapter_options used universally across all adapters, relying on the developer to pass in a proper configuration for their redis gem-of-choice. I don't think the hardcoded configuration limitations buy you anything, from what I can see.

cfis commented 5 years ago

Makes sense. Happy with your idea, can you code it up?