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

Provide some configuration DSL for custom Strategies and Locks #383

Merged
merged 7 commits into from
Apr 15, 2019

Conversation

mberlanda
Copy link
Contributor

👋 The value proposition of this pull request is to extend the DSL in terms of configuration.

The purpose is to reach something like:

SidekiqUniqueJobs.configure do |config|
  config.add_lock :some_lock, SomeLock
  config.add_strategy :some_strategy, SomeStrategy

  config.lua_cmd_paths << 'path/to/file.lua' # This may be extracted to another gem
end

This could be extended to Connections, Constants, Digests ...

I made this draft/poc out of a new class, but actually some options (:logger for example) are already supported in lib/sidekiq_unique_jobs/sidekiq_unique_jobs.rb SidekiqUniqueJobs::Config.

Before adding to this Concurrent::MutableStruct new elements, I would like make sure that this something suitable for @mhenrixon 😄

This may be related to a few open issues:
#184 , #328, #381

Custom Strategies, Locks etc would be available from the code where the gem is included (and we could contribute back to gem once we validated the approach on our projects first)

Copy link
Owner

@mhenrixon mhenrixon left a comment

Choose a reason for hiding this comment

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

I am really liking this approach. It has been on my mind forever how to extend and allow others to hook into and use the engine of this gem. Really happy to see some interest in making this happen. Since it wasn't requested before I've kept down prioritizing it.

spec/unit/sidekiq_unique_jobs/configuration_spec.rb Outdated Show resolved Hide resolved
lib/sidekiq_unique_jobs/configuration.rb Outdated Show resolved Hide resolved
lib/sidekiq_unique_jobs/configuration.rb Outdated Show resolved Hide resolved
@mberlanda
Copy link
Contributor Author

hey @mhenrixon thank you very much for your prompt review! I am going to rework my first commit to try to put everything into the SidekiqUniqueJobs::Config which exists already.

I will probably drop everything I put into 5991f30 at the end but I wanted to isolate the approach to make it easier to discuss

@mberlanda mberlanda marked this pull request as ready for review April 15, 2019 12:39
@mberlanda
Copy link
Contributor Author

mberlanda commented Apr 15, 2019

I came up with e747b4e for a complete review.

This pr introduces:

  • custom (on_conflict) strategies
  • custom locks
  • a separated configuration object

I managed to increase the overall code coverage.
I still would like to add an integration test to prove that it is actually working what I added.

I also found out a couple of configuration which may be overridden (e.g. SidekiqUniqueJobs::Digests::, Util::DEFAULT_COUNT, UtIl::SCAN_PATTERN) but I would leave the scope as it is to avoid a too large change. [It may be dangerous actually to provide too much flexiblity on those things]

@mberlanda mberlanda changed the title Draft for extending the configuration DSL Provide some configuration DSL for custom Strategies and Locks Apr 15, 2019
@mberlanda
Copy link
Contributor Author

@mhenrixon With the latest test pushed and a few doc more, I would consider this PR ready to review. I may add another commit to change the minor version if it is suitable for you but I am not sure about you handle the versioning of the gem.

Copy link
Owner

@mhenrixon mhenrixon left a comment

Choose a reason for hiding this comment

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

Looks great apart from class << self I'm not sure why but I'd like to keep it out of the code base as much as possible.

}.freeze

class << self
def default
Copy link
Owner

Choose a reason for hiding this comment

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

The only thing I immediately don't like is class << self. I've avoided it like the plague because I find that additional duplication of self very revealing. Especially in large classes where it is difficult to discern what level of nesting I'm at so please do use self.default. In my opinion not all duplication are to be avoided :)

I like having the default here and using it from the top level though! ❤️

}.freeze
# A convenience method for using the configured strategies
def self.strategies
SidekiqUniqueJobs.strategies
Copy link
Owner

Choose a reason for hiding this comment

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

I've used delegation a lot in many places but I honestly prefer a defined method and was planning on refactoring in this direction in some places 👍

@@ -31,4 +31,5 @@
require "sidekiq_unique_jobs/sidekiq_unique_ext"
require "sidekiq_unique_jobs/on_conflict"

require "sidekiq_unique_jobs/config"
Copy link
Owner

Choose a reason for hiding this comment

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

I like the move to a separate file now that it turned complex again! 👍

@mhenrixon
Copy link
Owner

I may add another commit to change the minor version if it is suitable for you but I am not sure about you handle the versioning of the gem.

When I release a new version I automatically bump to the next at the end of the release as to not forget about it so no need to bump anything.

You can check the rake task release for more info. It isn't perfect but it automates everything for me nearly every time.

@mberlanda
Copy link
Contributor Author

Actually thanks to this change we may define some custom on_conflict strategies specific to our project:

e.g. we use Datadog and we may want to increment a statsd metric instead of logging.
I am pretty sure other users have different vendors and they may want to extend their capabilities

@simonbaudry
Copy link

Nice improvement 👍

@mhenrixon mhenrixon merged commit a0c3f95 into mhenrixon:master Apr 15, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants