Flatten Arguments in Queue Payload #86
Conversation
markstory left a comment
markstory left a comment There was a problem hiding this comment.
I mentioned zero-downtime deploys a few times, and I think that is actually more important than developer facing backwards compatibility is. Code can be gradually upgraded, or in some cases fixed as one pull request. The in-flight messages of an application with more than a single webserver and queue worker will cross paths as deploying to multiple hosts/pods is not an atomic process.
Without a zero-downtime solution to the queue message format, we will require users to choose between:
- Never upgrading
- Requiring scheduled downtime.
- Losing data
All three of these sound like unfair requests when we as maintainers can choose to ignore it, or make other compromises to make the desired progress.
With that said, I'm not against requiring 'pit-stop' releases to make that zero-downtime upgrade work. For example the next release (0.1.4) could introduce the new format (but not remove the old). Then in 0.1.5 we can remove the old format. We can document this in an upgrade guide for the plugin and keep a list of what the pit-stops are for people who are upgrading.
src/Job/Message.php Outdated
| return $this->parsedBody['args'][0]; | ||
| return $this->parsedBody['args']; | ||
| } | ||
| | ||
| return Hash::get($this->parsedBody['args'][0], $key, $default); | ||
| return Hash::get($this->parsedBody['args'], $key, $default); |
There was a problem hiding this comment.
If we move forward with this, we should be able to handle old and new messages. This is important as during a deploy you'll have application instances emitting the old message format, and messages being consumed on a new worker process. By handling the old format and the new format we can provide users with a zero-downtime upgrade.
I would likely recommend using a different key than args for the new format. Because there will be argument lists that contain positional arguments a simple count check won't work. This will also necessitate emitting the args key as well because of cross talk during deploys.
There was a problem hiding this comment.
I see what you are saying. I'm not quite sure I agree that we need to provide a zero-downtime option for this since the package is still in beta, but I will take a stab at writing some code to support both.
There was a problem hiding this comment.
I went with the key data. I added a check to see if args exists and will use that. If args does not exist in the payload data is used. I updated all variables to be $data when dealing with this to denote the change.
There was a problem hiding this comment.
I see what you are saying. I'm not quite sure I agree that we need to provide a zero-downtime option for this since the package is still in beta, but I will take a stab at writing some code to support both.
Even though we're still in beta its good to think about the upgrade process, as if we can make it smooth and painless for folks who update frequently then they will continue to update.
src/QueueManager.php Outdated
| 'queue' => $queue, | ||
| 'class' => $callable, | ||
| 'args' => [$args], | ||
| 'args' => $args, |
There was a problem hiding this comment.
I agree that this wrapping array is a bit redundant. However, I'm concerned that there isn't a safe way to remove this without having duplicate data in the messages for at least a few releases.
Because $args can also be a positional list and have only one value eg. QueueManager::push(WelcomeMailer::class, [1]);. This scenario being functional right now, means we cannot reuse args and have a zero-downtime upgrade process.
amayer5125 force-pushed the flatten-args branch from e058c5d to ba50106 Compare src/Job/Message.php Outdated
| // support old jobs that still have args | ||
| if (array_key_exists('args', $this->parsedBody)) { | ||
| $data = $this->parsedBody['args'][0]; | ||
| } else { | ||
| $data = $this->parsedBody['data']; | ||
| } |
There was a problem hiding this comment.
What about new messages that are consumed by old workers? Wouldn't they still be missing their arguments?
There was a problem hiding this comment.
That is true. I guess for a short time we will need to double up the args and data in the payload until we remove args. I pushed an update.
amayer5125 force-pushed the flatten-args branch from ba50106 to d3de87c Compare 
args key is changed to data key to support backwards compatability
amayer5125 force-pushed the flatten-args branch from d3de87c to 65afd18 Compare 
| Thank you 👍 |
2 participants
We store arguments on the queue as an array, but there is only ever one element in that array. I can't think of a reason to have an array with only one element in it.
Old Structure:
{"queue":"default","class":["App\\Job\\HelloJob","execute"],"args":[{"name":"World","number":1,"bool":true}]}New Structure:
{"queue":"default","class":["App\\Job\\HelloJob","execute"],"data":{"name":"World","number":1,"bool":true}}