Closed rosstuck closed 9 years ago
I am going to repeat myself to have the complete response of mine here:
I really dislike the solution above as it introduces a responsibility which is:
ExecutedCommand
. CommandBus should know if the command has been executed before)P.S.: Hm, rereading this whole comment it seems like I started a war against your idea @rosstuck. While it's not my intention, it shows that I'm quite against the idea. Based on my experiences, if you need this functionality, you are doing something wrong. If you really want this implemented, it should go into a LazyQueueMiddleware
or something, because even if I accept that someone wants it, it should not be in the "core" plugin. But again, rather leave this whole thing. :stuck_out_tongue_closed_eyes:
Hehehe, no hard feelings! It's code, not personal. :smile:
I do think there's a key difference here: regardless of which machine the worker runs on, you're probably best off checking out the same codebase there, so your configs are centralized. It's about whether or not you run the code, not whether or not it's present. Lots of huge scale companies run with single massive codebases. So, the physical machine / configuration argument isn't relevant here, I think.
I don't think this breaks the implementation or responsibilities here, since the middleware is in charge of whether or not to queue or dispatch onwards. This is just a subcase of "dispatch onwards" to me.
I can see an argument for having two versions of this, but that needs to be considered carefully since this version relies on a change in the router as well (which isn't kind to that approach).
If I am correct than the root of our disagreement is that:
Infinite loop is only possible of you pass commands to the same queue. However if you want to pass every command to a secondary queue from the consumer (eg. a statistics of commands executed) you wouldn't be able to, since it comes from a queue. This is a good example of why I think having this limitation by default is a bad idea.
@rosstuck if the envelope is added BEFORE queueing in the LazyQueueMiddleware
than the router can be unaffected, thus the whole implementation can be in one place.
@sagikazarmark I suspect the root is who should decide whether a command is queued, the controller layer or the service layer.
How about I just send a PR on Thursday or Friday to see if we can compare and/or ship two implementations here? Because it seems we have two completely different takes on this issue and we'll talk in circles otherwise.
@rosstuck agreed, we have different priorities here and I think this question is already solved that there should be a solution for the mentioned problem, we are only arguing about the "default" behaviour.
Looking forward to see the PR.
Till then here is my solution mentioned earlier:
LazyQueueMiddleware:
/**
* {@inheritdoc}
*/
public function execute(Command $command, callable $next)
{
if ($command instanceof Message) {
if ($command instanceof QueuedCommand) {
$command = $command->getCommand();
} else {
$this->queue->enqueue(new Envelope(new QueuedCommand($command)));
return;
}
}
return $next($command);
}
class QueuedCommand implements QueueableCommand
{
/**
* @var Command
*/
protected $command;
/**
* @param Command $command
*/
public function __construct(Command $command)
{
$this->command = $command;
}
/**
* Returns the wrapped command
*
* @return Command
*/
public function getCommand()
{
return $this->command;
}
}
It makes the queued object graph more complex, but can be in a single class without caring about the router.
Finally got some time to think about this a little more.
I like how how your iteration above keeps it limited to one class. The naming is tricky (I don't want to call it Lazy because that usually refers to lazily loaded) but that could very well work.
One potential downside is that if someone ever switches from the wrapped version to the non-wrapped version, their pending, serialized commands would begin to fail because the non-wrapped version won't find a handler for the QueuedCommand
. We could ship a third middleware that does nothing except unwrapping but that gets a little weird.
Also, the more I think about it, I feel like the choice about using the same or separate command bus for the worker should be isolated to just a single place: which instance we passed to the Router. That should be the only part of the system that knows or cares, and while this is technically true, we have some mingled concerns around that now.
So, to resolve those, I have a crazy bizarre compromise to propose: what if we use your pre-wrapping version and require that you always put the QueueMiddleware at the front of your Command Bus, whether it's the same or separate bus that originally queued it? In other words, we always wrap the command.
Doing so has the following properties:
Downsides:
I know that sounds really unintuitive at first but if we're trying to eliminate the number of branching possibilities here, I think this might be the best compromise for everyone, especially the end users. Give it a once over and let me know what you think. :smiley:
Do you say that WE should care about whether a developer is stupid enough to exchange the middleware to something that should clearly not work? What other possible reasons can you mention for doing that?
The point of having two, separate implementations is that you can choose the behaviour you expect from the command bus. So far your argument was that end users want to use the same codebase. Why would anyone change it then?
I don't like the always wrap idea, because I don't like the wrap idea at all. I think it is a workaround for those who want to use "the same codebase". So I think this idea eases to use it like you would like to, but makes it harder how I would like to. What I proposed solves the problem as two equally important one. The fact that exchanging them ruins everything...well, choose a path and stick to it. It doesn't worth making one bad to make the other the ultimate one.
Regarding the router: it is just an interaction point with the consumer, it shouldn't be involved in any of these actions as it will never be aware of the command bus context.
As I said, the two problems should not be mixed up.
Either this or I don't completely get your idea.
Yes, we should care about making things easy for our users. Also, let's not call anyone stupid. :disappointed:
Here's the best reason I can give. Let's say we split this into two different middleware: QueueToSameBusMiddleware
and QueueToDifferentBusMiddleware
. If we split this into two middleware, say, we need to write this in the docs:
QueueToDifferentBusMiddleware
, make sure you configure and pass a separate bus to the Bernard Router as well.If we choose a single middleware approach, we need to write this in the docs:
There's a different burden of knowledge here on the end user and a lot fewer variations to support. You and I know a lot about the internals of how this stuff works, but it's not intuitive to folks who just want to use this and not worry about what's happening internally.
Does this place an extra burden on the QueueToDifferentBus
folks? Yes. We are adding the requirement to have one extra middleware in their stack up front. But I think it's a pretty low burden, honestly, and it will be easier for us to support in the future since there's fewer variations possible here (both in code and queue state).
Finally, to gain market share, we need to be really easy to use. The framework integration packages are going to lean towards a single command bus solution, so we should lower the barrier for that, offering users a plug'n'play solution without worrying internally how things are implemented.
As a separate or alternate though, if this really boils down to the wrap, we could consider a base class/interface/trait setup that allows setting a mutable flag (isAlreadyQueued
) that eliminates that need for the wrap and middleware on the other side. This also allows other middleware to see the true command without having to peel the wrapper first, so it doesn't always need to be first in the list.
It does require tampering with the interface on the class though and a little more setup per command.
Still, that could work too and might solve everything.
Also, gonna ping @andrewcairns as well because he's actually dealing with this use case in practice and I'd like to hear his thoughts on either of these solutions. :)
Your argument is that we have to document more features if we have two separate things. This is true, but you are proposing to have only one feature and call the other a workaround. Also, I don't think having to document more things is a valid argument, because the need of documentation should not have effect on features.
I don't really understand the easy to use and market share arguments. You have one "easy" way to use and there is another. Whichever you choose, your call. What is so difficult about it?
If you remember, I wanted to have a way to control from the code whether a command should be sent to a queue or not. Your point then was to have an interface or something to make this decision. Now we changed places. ;)
I am gonna try to get something out of this, because we are not going to move from one to two otherwise. Let me think about a possible solution.
I can imagine an always wrapping logic, but in this case, this wrapper also has to be the decision if the command should be queued or not. In other words: having a QueueableCommand
interface and a QueuedCommand
is redundant. We should choose one of them.
In this case, we should either make it necessary to pass a wrapped command to the queue, or if the QueueMiddleware
is in the bus, we have to catch all commands and if it is not from a queue it should be sent to one. In other words: either wrap by the user, or send all commands to the queue by default.
I made a PR which probably solves all you issues. However I make one last objections: this is WRONG. We should support both options, because the same codebase and all other things are one possible way to do things. The way I do things is an other. None of them is better than the other, simply different thus we should support both way. This idea basically forces me to do things how I wouldn't like to and I have bad feelings about it.
So I will probably remove the PR and let you do what you want.
Take your time, study them all and choose the last one. ;) I don't even care if you document only that one.
Hey, sorry for the late responses, I've been onsite with a remote client all week.
Crazy awesome work on the PRs. :smiley_cat:
To clarify, I'm okay with either of the last two so pick your fave and we'll go from there
Actually I realized that if we support BOTH in one middleware, it might not be a problem. So my vote goes to #13.
However I am not sure if it makes any difference, since we have to document two routers instead of one.
While admitting that neither of us really liked #12, in light of discussions on #15, what would you think about an interface like this?
interface QueueableCommand / CanBeQueued
{
public function getQueueStatus();
public function changeQueueStatus();
public function getQueuePriority();
public function getQueueName();
}
We could also supply a trait that has all the necessary code and returns Priority/Name default values as well. I also dislike the mutability of the QueueStatus but if there are going to be other options, this might be the simplest way to resolve all of that in one clean pass.
Just throwing it out there. :smile:
I have multiple reasons against it:
Command
and Bernard's Message
directly. In other words: if you implement those interfaces in your command, you can still use it. And it is nicely aligned with SRP.From the other side, rather this than wrapping the command, because the middleware architecture could really be messed up with that solution. (Actually I am thinking about a general command envelope pattern used all over in tactician, which could generalize the need for envelops and ease the usage of them, but that is just an idea).
Earlier this week, we had a quick chat about the new Router replacement system and how that could create an infinite queuing loop if given the same command bus.
I'd rather not force people to have two command buses because:
@sagikazarmark's earlier response was that this might cause more bootstrapping than folks intend for a light-weight worker, but I don't think that's a given and is more a choice of DI container/framework than Tactician's responsibility.
Anyways, I was thinking about ways of resolving this. The best idea I've had so far is that we keep the initial queuing the exact same (user drops a QueuableCommand in, that gets serialized, etc). Then on the worker side, it gets popped out of the queue and sent to the router.
Here's where the two changes start: rather than returning
[$this->commandBus, 'handle']
we returnThen in the QueueMiddleware, we add an extra if statement to avoid the loop:
In short, we signal the state to the QueueMiddleware by wrapping the Command in a QueuedCommand wrapper. That prevents the infinite loop and lets us use one command bus everywhere. It's a little bit "different" for the internals but easier for the consuming user (provided they take our advice about putting the QueueMiddleware always go at the top of the list).
Thoughts?