-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
transactional_push! prevents jobs from being added to a batch #6160
Comments
We can either guarantee that Batch jobs are pushed atomically at the end of the |
Heya @mperham! Long time no see. Is this a case of "we're using it wrong" or is there a real question here? |
I apologize for the less than useful initial reply. It looks like the transactional push delay causes the batch to go out of scope and so the job does not get a BID associated. That seems like a footgun I should handle. 👍🏻 |
Clarified the description a little bit, I hope that helps. Ideally the jobs would still be pushed at the end of the transaction, but they would get their BID assigned. If it's not possible to do both, then I guess it's more important for a BID to be assigned than to enqueue at the end of the transaction. |
Somehow this works for me. I can't reproduce the behavior yet. Can you use 7.2.0? it "works transactionally" do
q = Sidekiq::Queue.new
assert_equal 0, q.size
require "active_record"
ActiveRecord::Base.establish_connection(
adapter: "sqlite3",
database: "test.db"
)
require "after_commit_everywhere"
Sidekiq.default_job_options["client_class"] = Sidekiq::TransactionAwareClient
Sidekiq::JobUtil::TRANSIENT_ATTRIBUTES << "client_class"
class XAJob
include Sidekiq::Job
end
ActiveRecord::Base.transaction do
b = Sidekiq::Batch.new
b.jobs do
XAJob.perform_async
end
assert_equal 1, q.size
end
assert_equal 1, q.size
end |
I suspect the batch refactoring in 7.1.x fixing inline execution also changed this behavior. |
Try main and see if it works better for you. |
The problem still exists on main. The test you wrote isn't testing the issue I mentioned, which involves the batch id not being set. I'm also not convinced this test passes, given that it's trying to use transactional push, I would expect the I would rewrite it as:
the last assertion is still broken on main. |
@mperham sorry it took me a while to respond, wanted to make sure you saw this? ^ |
Are you using Sidekiq + Sidekiq Pro 7.2.0? 7.1.0 will not work. |
Batch jobs do not pay attention to transactional boundaries. The ActiveRecord::Base.transaction do
b = Sidekiq::Batch.new
b.jobs do
XAJob.perform_async
end # the job will be pushed here
assert_equal 1, q.size
assert_equal q.first.klass, 'Sidekiq::Batch::Empty' # nope, this will be XAJob
end
assert_equal 2, q.size # nope, only one job
assert_equal q.map{|j| j.klass}, ['Sidekiq::Batch::Empty', 'XAJob']
assert_equal q.map{|j| j.bid}, [b.bid, b.bid] # and yes, you should get a bid |
Is this documented somewhere? This is surprising to me and also does not match the behavior I'm seeing, in both 7.1.0 and 7.2.0 With Sidekiq + Sidekiq Pro both on 7.2.0 what I'm seeing is:
|
As a side note, with 7.1.0 I am not seeing the
|
You need Sidekiq main + Pro 7.2. |
🤦 I didn't realize main was ahead of 7.2.0 and included this fix. My bad 😬 Looks like that resolved the issue! I will (ask our infra team to) update when it gets released. Thank you! |
@mperham I'm working on updating sidekiq and bump into this issue ((isolator alerted us of pushing to jobs while inside a transaction). Our scenario is as follows:
For the pushing after the transaction part I believe we can add It seems that side effect of that would be that those jobs would be hanlded outside the batch. Is this assumption correct? I think the uniqueness of our scenario is that we have jobs that we care to track as part of the batch and some jobs that we don't. Idealy we would be able to provide a list to the bach of what classes are part of it and it would only track those instead of assuming every job pushed in the context of a batch is part of it. pd: sorry to post on a closed issue, not sure if that was the way to go or if I should have opened a new one |
We cannot provide both guarantees at the same time. Here's an idea: do the work to separate those two concerns. Either commit your transactions before creating the batch or scope the transaction within the Commit in batch: batch.jobs do
User.transaction do
...
end
end Commit then batch: User.transaction do
...
end
batch.jobs do
end |
We don't have a problem or a transaction at the initial level. We do:
with the latest changes
but thats kind of annoing since the call for that worker might be nested insde a bunch of method calls (and sometimes its called without a batch being present in those cases we wouldn't need the |
Just to close the loop, turns out that pushing jobs from a job inside a batch DOES NOT push the jobs to the batch automatically that was happening on our test because of some workarounds we have to be able to perform integration tests on jobs that depend on batches. Sorry for the noise! |
Ruby version: 3.2.2
Rails version: 7.0.8
Sidekiq / Pro / Enterprise version(s): 7.1.0 / 7.1.0 / 7.1.0
Using the
transactional_push!
featurein sidekiq initializer
When adding jobs to a batch inside a transaction, the enqueued jobs are not associated with a batch.
The text was updated successfully, but these errors were encountered: