fix(que): Fix bulk_enqueue when enqueuing more than 5 jobs #1074
Conversation
laurglia
requested review from
fbogsany,
mwear,
robertlaurin,
dazuma,
ericmustin,
arielvalentin,
ahayworth,
plantfansam,
robbkidd,
simi,
kaylareopelle and
xuan-cao-swi
as code owners
|
|
|
|
||
| unless job_attrs.empty? | ||
| span.name = "#{job_attrs.first[:job_class]} publish" | ||
| span.add_attributes(QueJob.job_attributes(job_attrs.first)) |
There was a problem hiding this comment.
Does it add messaging.message_id to the attributes? It shouldn't in case of a bulk enqueue. The other properties seem correct because they should be the same according to the bulk enqueue limitations.
There was a problem hiding this comment.
The job attributes do not contain ID in the case of bulk enqueue, so no ID is added to attributes.
This is so because IDs only exist after a job is inserted into database but for bulk enqueue we set the attributes before they are inserted into database.
There was a problem hiding this comment.
I will see if I can also add a test to assert that message ID is not added
There was a problem hiding this comment.
Added a test case to check lack of message ID attribute and presence of other attributes
| @@ -363,10 +416,10 @@ def self.run(first, second); end | |||
|
|
|||
| _(finished_spans.size).must_equal(2) | |||
|
|
|||
| span1 = finished_spans.first | |||
| span1 = finished_spans.last | |||
There was a problem hiding this comment.
did something change with ordering?
There was a problem hiding this comment.
Yes, forgot to mention this in commit message. It affects the "synchronous" mode test where previously the "produce" span finished first and only then "consume" finished. Now they finish in opposite order.
I do not know what is the desired behavior, not sure if it's even important here what order they are in.
But observe that now the behavior is identical to the normal enqueue for synchronous jobs. Here is same test for regular enqueue and observe that now the two tests are identical.
|
Hi @laurglia! Thanks for your contribution. Would you mind signing the CLA? |
I am working on it. As I'm making this under employment with a company, I will need to first get the company sign the CLA. |
|
👋 This pull request has been marked as stale because it has been open with no activity. You can: comment on the issue or remove the stale label to hold stale off for a while, add the |
|
Hi @laurglia! Any luck with the CLA? Is there anything we can do to help? |
laurglia
force-pushed
the
fixbulkenqueue
branch
from
7c972ab to
66f497c
Compare
The CLA has been signed. |
laurglia
force-pushed
the
fixbulkenqueue
branch
from
66f497c to
176a1bb
Compare
Previously, with Que `bulk_enqueue` a new span was created for every job in the batch. For each such span, a tag was added to _all_ jobs in the batch. This meant that if you enqueued 3 jobs, then 3 spans were created and each job had 3 tags pointing to 3 different spans. This behavior became problematic when you tried to insert more than 5 jobs, as Que supports up to 5 tags per job. More specifically, a runtime error was raised by Que. This commit fixes that bug by creating only a single span when using bulk_enqueue.
laurglia
force-pushed
the
fixbulkenqueue
branch
from
176a1bb to
db79b63
Compare
Previously, with Que
bulk_enqueuea new span was created for every job in the batch.For each such span, a tag was added to all jobs in the batch. This meant that if you enqueued 3 jobs, then 3 spans were created and each job had 3 tags pointing to 3 different spans.
This behavior became problematic when you tried to insert more than 5 jobs, as Que supports up to 5 tags per job. More specifically, a runtime error was raised by Que.
This commit fixes that bug by creating only a single span when using bulk_enqueue.