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.

Sign up for GitHub

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

Fixes #224. I believe this is a Postgres only issue #231

Merged
merged 4 commits into from
Aug 6, 2024
Merged

Fixes #224. I believe this is a Postgres only issue #231

Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
cf4e62e
Fixes #224. I believe this is a Postgres only issue
hms May 22, 2024
1d115b6
Address rubocop unhappiness with single vs. double quoted strings
hms Jun 28, 2024
c817349
Fixes #224.
hms Aug 5, 2024
7d36912
Cleans up edited files to project standard formatting
hms Aug 5, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
Address rubocop unhappiness with single vs. double quoted strings
  • Loading branch information
@hms
hms committed Aug 5, 2024
commit 1d115b6e147a23ec130998cf60797453725cef06
2 changes: 1 addition & 1 deletion app/models/solid_queue/semaphore.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,7 @@ def attempt_creation_with_create_and_exception_handling
limit == 1 ? false : attempt_decrement
end

if ActiveRecord::Base.connection.adapter_name == 'PostgreSQL'
if ActiveRecord::Base.connection.adapter_name == "PostgreSQL"
Copy link
Member

Choose a reason for hiding this comment

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

It seems there's a method that checks directly whether the adapter supports unique_by exposed in Active Record's Abstract adapter, that we could use simply as

connection.supports_insert_conflict_target?

I think this might be more robust than checking the adapter name, because other adapters for PostgreSQL might have a different name, like the PostGIS adapter.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So funny and just goes to show a bit of tunnel vision. I spent a fair amount of time digging around for an alternative to the original nested transactions methodology and in that digging I came across this method in the Rails adaptors.

The tunnel vision part was no thinking to use those methods as the much, much better way to test which of the code paths to take.

Thanks for this -- it's a great reminder to slow down and think before coding.

alias attempt_creation attempt_creation_with_insert_on_conflict
else
alias attempt_creation attempt_creation_with_create_and_exception_handling
Copy link
Member

Choose a reason for hiding this comment

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

I'm thinking that this all could perhaps live in the Semaphore class in a simpler way. Something like this method:

diff --git a/app/models/solid_queue/semaphore.rb b/app/models/solid_queue/semaphore.rb
index 34bf8dc..1448e67 100644
--- a/app/models/solid_queue/semaphore.rb
+++ b/app/models/solid_queue/semaphore.rb
@@ -17,6 +17,17 @@ module SolidQueue
       def signal_all(jobs)
         Proxy.signal_all(jobs)
       end
+
+
+      def create_unique_by(attributes)
+        if connection.supports_insert_conflict_target?
+          insert(attributes, unique_by: :key).any?
+        else
+          create!(attributes)
+        end
+      rescue ActiveRecord::RecordNotUnique
+        false
+      end
     end

     class Proxy

Copy link
Member

Choose a reason for hiding this comment

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

Then that could be used in Proxy class like this (BTW I might remove that class in the future and leave everything in Semaphore... it was not to have to pass all the job attributes around, but still... anyway, not the goal of this PR):

        def attempt_creation
          if Semaphore.create_unique_by(key: key, value: limit - 1, expires_at: expires_at)
            true
          else
            check_limit_or_decrement
          end
        end

        def check_limit_or_decrement
          if limit == 1 then false
          else
            attempt_decrement
          end
        end

And you wouldn't need to define aliases.

Expand Down