search

marcoCasamento / Hangfire.Redis.StackExchange

HangFire Redis storage based on original (and now unsupported) Hangfire.Redis but using lovely StackExchange.Redis client
Other
456 stars 109 forks source link

Configurable prefix #6

Closed VoidMonk closed 9 years ago

VoidMonk commented 9 years ago

Hi,

I noticed that you're adding batches support in a dev branch. It would be nice if you can also add support for configuring the key prefix like the original (http://docs.hangfire.io/en/latest/configuration/using-redis.html#using-key-prefixes) and this fork (https://github.com/jamesjhhan/Hangfire.Redis.StackExchange).

Cheers :+1:

marcoCasamento commented 9 years ago

Yes, I Agree. It will be on the next release. I'd welcome a PR from this rel btw, I'll contact @jamesjhhan

jamesjhhan commented 9 years ago

Hi Marco. I just did a PR. The only caveat to my solution is that setting/updating the key prefix is not technically thread safe. However, practically you just set at the app startup and never update it. I can't see a case where an application would need to update it outside of the app startup.

marcoCasamento commented 9 years ago

I see your point and agree that changing that settings at Runtime doesn't make sense. It would however make sense to leave the field Prefix in the Storage class as readonly. do you agree ?

jamesjhhan commented 9 years ago

The problem with making it readonly is that then I can't set the value from the RedisStorage constructor. I'd need to make it non-static and then I'd have to do a lot of refactoring. If I am missing something obvious, please feel free to let me know.

VoidMonk commented 9 years ago

I agree with jamesjhhan, it should be a constructor setting (also to maintain consistency with the official Hangfire.Redis lib - see http://docs.hangfire.io/en/latest/configuration/using-redis.html#using-key-prefixes).

marcoCasamento commented 9 years ago

Yes, you 're right, I didn't realize that field is referenced 106 times in the code! It would be a lot of work for a very little result, so I agree with you, let it be as is now.