Hi Alena,
2014-02-26 23:19 GMT+01:00 Alena Prokharchyk <alena.prokharc...@citrix.com>:
> Antonio,
>
> I still don't quite see why false result returned from one of the
> workers, should silently stop other workers if those workers are a)
> independent b) have their own way of processing the failures in
> asynchronous manner later as you stated.
>
If you don't see why to stop the chain then you can just return true. On
the other hand, the Chain of Responsibility Pattern from GoF always has
been implemented in a way the each worker can invoke the next OR NOT. And
again, this is very common in JEE filters, I'm sure you already found
similar examples with JEE filters, right?
Yes, workers are independent. And one worker can be designed to stop the
chain under certain conditions even if this worker doesn't have a clue what
workers come later. Then you decide to put this worker in the chain before,
after or not to put it at all. Just like in CoR pattern from GoF.
On the other hand, removing this flexibility has any benefit? You keep
saying "silently", but not invoking a method that should not be executed is
not something silent. That's because you still see false as "error" and not
as "mission complete".
In general I prefer to choose non restricting choices. Just like I USUALLY
prefer protected members over private ones, because in general I assume if
another developer in the future decides to extend a method she will do it
for reasons... even if I don't see the reasons in advance. I can also make
the method private, but that only causes the developer will change the
method to protected and then extend it when needed, speaking about this...
... I really don't see is this conversation changing our points of view. I
prefer to just return void, if anybody need to change this in the future
they can always do it.
> You also say "didn't want each worker to have a reference to the next.".
> But in your case you do obey the reference in indirect when failure of one
> worker execution affects the other? I'm confused.
>
Didn't want each worker to have a reference to the next, but not because of
decoupling, just because if WorkerA -> WorkerB then I cannot have a chain
WorkerA -> AnotherWorker unless I create another instance of WorkerA
(because a worker object only has one next). So my point was, better to
create only one instance of each worker given that there will be not
benefit on having each worker referencing the next. I never meant it was
because of encapsulation...
...because of encapsulation we do other things, but not that. We deal with
workers by interface, but this is something you have whether you use list
or wrappers, whether you return void or boolean. Having workers in a chain
make it flexible and modular, but doesn't make imperative that the workers
cannot affect each other or assume anything about others. Your assumptions
in a worker should be documented so it's easy to know how to create the
chain. something like: "This worker stops the chain if we decide to put the
cmd in a queue due to X". You have to keep in mind how they affect each
other in order to decide how to create the chains: that knowledge is the
role of the factory.
And actually the previous code (and many others) are like that: in a
certain block of code sometimes you do something considering the code that
may come later. And actually we do it in the workers: one of the workers
(ParamUnpackWorker) changes the parameter format assuming that next workers
will need the new format instead of the old one. Then it's up to you to
keep workers in order: if you don't put this worker in the first place in
the list then the other workers will fail because the params are not
formatted. Thus this worker knows that the next workers will never get to
the old format params... and that also happened before with the
unpackParams method, although the method itself doesn't know all details
about what will happen with the new format params later.
>
> As an example, we can look at couple of examples of something similar in
> CloudStack code.
>
> 1) SecurityChecker Adapters. All adapters are invoked on the resource,
> and if something fails, it throws an exception that clearly stops others
> checkers from execution.
>
In Spring Security you can configure several "agents" to decide to
authorize access or not, and you can configure it to work if all the agents
say YES or if AT LEAST ONE says yes. In the latter case, the moment one
gives the YES it does stop the others from doing a check. You can find 20
examples in which you don't need to stop, but if you find a single example
in which you do need it that should be enough. Why to remove the
possibility just because you don't come across a good example? Why to keep
it more restricted instead of more flexible?
> 2) UserAuthenticator Adapter. When user logs in, all adapters defined in
> the config file, get invoked in specific order. If one of them fails
> (RuntimeException is thrown), the login attempt fails as well, and other
> adapters are not invoked.
>
But this is an example related with failure and I was never speaking about
failure. Returning false was never a replacement from raising exceptions.
ie: The method MyDao#countEntities returns a number and throws and
exception if there are connection problems. Would you be afraid that
someone will return -1 instead of throwing an exception? No, because the
return is not used for that. And in the Dispatch Chain returning false
doesn't mean "I return false because I hide a RuntimeException", but more
"I receive a DispatchTask that I know is specifically for me, so I
consume/process it and stop the chain because I've been designed for that
purpose".
>
> In other words, if you decide that each worker returns boolean variable,
> it means that you have to process that result in the caller stack, which
> the code-to-review doesn't do. You either:
>
:-) No! Above all the caller doesn't process anything because the details
of the current chain may be transparent for the caller. The caller puts
milk in the coffe machine and trusts the coffee will come, and he doesn't
mind how many pipes the milk went through. Exceptions are different, and
that would be the callers business. But the false/true is only for the
iterator to decide if the work is done, not for the caller. And that's why
the method DispatchChain#dispatch is void, although the internal method
Worker#handle returns boolean. The caller doesn't mind the boolean, the
iterator (DispatchChain) does.
> * throw RuntimeException when one of the workers returns failure. It
> would eliminate your fear that developer writing the worker, will forget to
> do it in handle() method itself
>
Exceptions are apart from the subject, because I never return false to
avoid throwing an exception (actually I never return false at all but I
design it to be open).
* change the return type to void. I'm pretty sure the developers writing
> the workers, will take care of throwing an exception on validation failure
>
Of course they will do, whether we go left or right. I never thought a
developer would return a false instead of throwing an exception just
because he can. His patch would not pass the review :) And further he would
read the javadoc header
* @return false to stop the chain of responsibility, true
* to continue the chain with the next worker
...nothing mentions that false means error.
> Whatever one you prefer from the above, I'm fine with.
>
Actually I'm fine with doing the changes you tell me in your next answer,
but I cannot choose between these 2 options because the code never returned
false in case of any error.
I really want to have this patch through. I will do whatever changes you
tell me in your next mail, and if you don't mind I will request your review
for future related patches. Can you confirm what other things you finally
want me to change in addition to this?
Thank you very much again.
Antonio
> Let me know what you think,
> -Alena.
>
> From: Antonio Fornié Casarrubios <antonio.for...@gmail.com>
> Date: Wednesday, February 26, 2014 at 2:02 PM
> To: Alena Prokharchyk <alena.prokharc...@citrix.com>
> Cc: "dev@cloudstack.apache.org" <dev@cloudstack.apache.org>, daan
> Hoogland <daan.hoogl...@gmail.com>, Hugo Trippaers <
> htrippa...@schubergphilis.com>
> Subject: Re: Review Request 17888: Dispatcher corrections, refactoring
> and tests. Corrects problems from previous attempt
>
> Hi Alena,
>
> Answer inline:
>
>
>
> 2014-02-26 22:24 GMT+01:00 Alena Prokharchyk <alena.prokharc...@citrix.com
> >:
>
>> Antonio, please see my 2 comments inline.
>>
>> -Alena.
>>
>> On 2/26/14, 1:17 PM, "Antonio Fornié Casarrubios"
>> <antonio.for...@gmail.com> wrote:
>>
>> >Hi Alena,
>> >
>> >I answer to your comments inline:
>> >
>> >
>> >2014-02-26 19:08 GMT+01:00 Alena Prokharchyk
>> ><alena.prokharc...@citrix.com>:
>> >
>> >> This is an automatically generated e-mail. To reply, visit:
>> >> https://reviews.apache.org/r/17888/
>> >>
>> >> Antonio, in general looks good to me. There are some minor fixes that
>> >>need to be done though.
>> >>
>> >> 1) ApiServer.java
>> >>
>> >> @Inject()
>> >> 177
>> >> protected DispatchChainFactory dispatchChainFactory = null;
>> >>
>> >>
>> >> Why do you use @Inject with ()?
>> >>
>> >>
>> >*I tried some parameters with the Inject annotation but finally I
>> decided
>> >to use it as it is here. Just a habit from using other types of DI
>> >annotations. I just didn't didn't remove the () in this one, but as you
>> >know it's exactly the same so it didn't even bring my attention. I can
>> >change that or if you approved this patch, change it in the next patch.
>> >Again, it's just the same.*
>> >
>> >
>> >
>> >> 2) ApiServlet.java
>> >>
>> >> domainIdArr = (String[])params.get(ApiConstants.DOMAIN__ID);
>> >>
>> >> * Why do we have DOMAIN__ID and DOMAIN_ID as separate parameters?
>> >>
>> >> public static final String DOMAIN_ID = "domainid";
>> >> public static final String DOMAIN__ID = "domainId";
>> >>
>> >> The above doesn't look right to me. Why do we care about the case? the
>> >>API parameter is always transformed to all lowercase letters
>> >>
>> >> *Having these almost identical Strings is not my change, ApiServlet
>> was
>> >already using both with I and i (domainid and domainId) so I just kept it
>> >as it is (I assume removing one of them may cause a problem when it comes
>> >in the req. In order to create the constants for both values I just chose
>> >these names: no names would make much more sense because having this two
>> >Strings were already something strange. There are several strange things
>> >like this, I just fixed a few and moved plenty from hardcoded values to
>> >constants, but no more than that, my main focus was something else and
>> the
>> >patch is already very big.*
>> >
>> >3) CommandCreationWorker.java
>> >>
>> >>
>> >> throw new ServerApiException(ApiErrorCode.INTERNAL_ERROR,
>> >> 50
>> >> "Trying to invoke creation on a Command that is not
>> >> BaseAsyncCreateCmd");
>> >>
>> >>
>> >> * Don't hardcode the class name; retrieve it from the .simpleName
>> >>method called on the class object
>> >>
>> >> *Retrieve it? This is a piece of code that was in
>> >ApiDispatcher#dispatchCreateCmd just after calling processParameters, so
>> >it
>> >had to be a next worker in the chain, but I kept the code as it was. And
>> >this specific method was only invoked after checking the command was
>> >BaseAsyncCreateCmd (or any class extending, of course), so I created the
>> >same check in the worker. And then of course I keep also the reason to
>> >fail
>> >in the error msg. If it fails the only thing I care is that it is not a
>> >BaseAsyncCreateCmd as it should... so I don't understand what do you
>> >propose there. For anybody who chages this code in the future, this error
>> >will tell them they should not apply this worker when the dispatch chain
>> >is
>> >not the expected type.*
>>
>>
>> :) What I¹ve meant is, instead of "Trying to invoke creation on a Command
>> that is not BaseAsyncCreateCmd², log it as "Trying to invoke creation on
>> a Command that is not ³ + BaseAsyncCreateCmd.class.getSimpleName(). So in
>> case someone changes the class name in the future, you don¹t have to dig
>> in into hardcoded stuff in order to change it, as it will change
>> dynamically
>>
>>
> *** I see, sorry I dind't understand you meant that. I will sure change
> that, good point.
>
>
>
>>
>> >
>> >> 4) DispatchChain
>> >>
>> >> for (final DispatchWorker worker : workers) {
>> >> if (!worker.handle(task)) {
>> >> break;
>> >> }
>> >> }
>> >>
>> >> * Why do we just break when workder can't handle the task? Aren't we
>> >>supposed to do something?
>> >> * Can you please add more logging? At least log in trace that some
>> >>worker handled/coudln't handle the task
>> >>
>> >>
>> >> *There are many ways to create a Chain of Responsibility, for this
>> case
>> >>I
>> >decided to go for a one way chain (instead of a typical two way flow) in
>> >which workers share a task where they apply their work and pass their
>> >changes. They also have the power to stop the chain if the programmer
>> >decides so in her code. I'm not assuming the worker can't handle anything
>> >or not not doing something, I am just making sure I provide a way to stop
>> >the chain if one of the workers decides so. If there were something to
>> >log,
>> >the worker would know what and if something requires so I guess they will
>> >raise and exception instead of just returning false. We don't have
>> >anything
>> >to log just because the behavior was to stop the chain. Just see it as
>> JEE
>> >filters where they can decide to do whatever even there is no error.*
>>
>>
>>
>> Stopping and doing nothing after all doesn¹t look right to me, as you
>> don¹t pass the result to the caller stack to process it further depending
>> on the return type. I would rather change work.handle() return type to
>> void. As as you said, worker would throw an exception anyway if something
>> goes wrong.
>>
>>
> *** There is no "Stopping and doing nothing", you create your chains
> considering the order of workers and you code the workers considering what
> you want to do in the chain. You should be free to consider the work done.
> Just like you decide to execute some code or not with an if, and that
> doesn't mean you are doing nothing: there is nothing to do if the clause
> for the if is not true: this is the same. ie: Shouldn't a worker be able to
> decide to save the cmd for later execution asynchronously and when
> retrieved from a queue go through another chain? I don't know if this is a
> good example, but actually for something similar we are using ifs and
> instanceof checks to choose the flow, right? Again the JEE filters are the
> best example and they are also used to process/dispatch a request/cmd.
>
> The reference implementation of Chain of Responsibility is based on
> workers that wrap the next one recursively (like JEE filters) and thus each
> worker is free to invoke the next worker or not. With your proposal we
> loose that. Actually I prefer to go for the that reference implementation,
> but I decided to go for a list with boolean return types because: 1) We
> don't need pre and post process, it's a one way chain (at least so far) and
> also because 2) I want to create the workers as Spring singletons (default
> scope), but I also want to be able to have several chains with different
> workers so I didn't want each worker to have a reference to the next.
> Someone could could brake it by returning false when you shouldn't just
> like you could by throwing an exception when you shouldn't, but that's not
> a reason not to give this flexibility to the workers. I think it's better
> to assume that new workers will be coded appropriately.
>
> By the way and in case this info helps (I will include it also in the
> FS), some more clarification: The old way in the ApiDispatch was this way:
>
> hugeMethod(){
> // Some code for doing A
>
> // Some code for doing B
>
> if (cmd instanceof CmdTypeX) {
> // Some code for doing C
> }
>
> if (N) {
> } else {
> // some code for doing D
> }
> }
>
>
> And the new approach is:
> Chain for X
> A > B > C
> Chain for N
> A > B > D
>
> So in the end what you could decide to do based on ifs or anything else,
> you can decide now based on the chain workers and on stopping the chain or
> not.
>
> For the next changes I plan to do more refactoring in this direction. I
> also did another kind of change so that instead of doing instanceof checks
> so often (we have them everywhere in CS code) we use polymorphism. For this
> I created the SemanticValidationWorker so that there the work is actually
> delegated to the Cmd class (polymorphism)
>
>
> >
>> >
>> >>
>> >> 4) I can see that you do a lot of initialization like that for private
>> >>variables:
>> >>
>> >> protected AccountManager _accountMgr = null;
>> >>
>> >> its not necessary step as private variables are being initialized to
>> >>NULL by default during declaration. Can you please clean it out?
>> >>
>> >>
>> >>
>> >*I know. In Java local variables are not initialized, but instance ones
>> >are
>> >(booleans to false, numbers to cero, objects to null...). But I find it
>> >more clear to put it this way, further people may ignore or forget it,
>> >specially non Java people helping in cloudstack. It's also something I've
>> >seen in many Java projects. Again, it's just the same. If you think this
>> >is
>> >not clean and should be changed before the patch is accepted, I can
>> change
>> >that too.*
>> >
>> >*Upon reception of your comments I will change and upload anything.*
>> >
>> >- Alena Prokharchyk
>> >>
>> >> On February 26th, 2014, 9:12 a.m. UTC, Antonio Fornie wrote:
>> >> Review request for cloudstack, Alena Prokharchyk, daan Hoogland, and
>> >> Hugo Trippaers.
>> >> By Antonio Fornie.
>> >>
>> >> *Updated Feb. 26, 2014, 9:12 a.m.*
>> >> *Repository: * cloudstack-git
>> >> Description
>> >>
>> >> Dispatcher corrections, refactoring and tests. Corrects problems from
>> >>previous attempts that were reverted by Alena. Most of the changes are
>> >>the same, but this one is not creating conflicts of Map types for Aync
>> >>Commands or for parameters as Lists or Maps.
>> >>
>> >> Testing
>> >>
>> >> Full build and test plus manually testing many features. Also including
>> >>CreateTagsCommand that failed in previous commit.
>> >>
>> >> All unit and integration tests.
>> >>
>> >> Test CS Web UI with the browser going through several use cases.
>> >>
>> >> Also use the CS API by sending HTTP requests generated manually
>> >>including requests for Async Commands with Map parameters and during
>> >>these tests apart fromtesting correct functionality I also debugged to
>> >>check that Maps created correctly where they should but also that in the
>> >>cases where the async command must be persisted and later on retrieved
>> >>and deserialized by gson everything works ok and does what and where is
>> >>expected. An example based on the comment by
>> >>Alena:
>> http://localhost:8096/client/api?command=createTags&resourceids=ids
>> >>&resourcetype=type&tags[0].key=region&tags[0].value=canada
>> >> Also other examples
>> >>like
>> http://localhost:8096/client/api?command=createSecondaryStagingStore&;
>>
>> >>url=httpbla&details[0].key=region&details[0].value=canada&details[1].key=
>> >>element&details[1].value=fire
>> >>
>> >> Diffs
>> >>
>> >> - api/src/org/apache/cloudstack/api/ApiConstants.java (7b7f9ca)
>> >> - api/src/org/apache/cloudstack/api/BaseCmd.java (0e83cee)
>> >> - api/src/org/apache/cloudstack/api/BaseListCmd.java (c1a4b4c)
>> >> -
>> >>api/src/org/apache/cloudstack/api/command/admin/user/GetUserCmd.java
>> >> (592b828)
>> >> -
>> >>api/src/org/apache/cloudstack/api/command/admin/user/UpdateUserCmd.java
>> >> (de6e550)
>> >> -
>>
>> >>api/src/org/apache/cloudstack/api/command/user/autoscale/CreateAutoScaleV
>> >>mProfileCmd.java
>> >> (06d86ec)
>> >> -
>>
>> >>server/resources/META-INF/cloudstack/core/spring-server-core-misc-context
>> >>.xml
>> >> (fd2f5fb)
>> >> - server/src/com/cloud/api/ApiAsyncJobDispatcher.java (71ac616)
>> >> - server/src/com/cloud/api/ApiDispatcher.java (ed95c72)
>> >> - server/src/com/cloud/api/ApiServer.java (d715db6)
>> >> - server/src/com/cloud/api/ApiServlet.java (46f7eba)
>> >> - server/src/com/cloud/api/dispatch/CommandCreationWorker.java
>> >> (PRE-CREATION)
>> >> - server/src/com/cloud/api/dispatch/DispatchChain.java
>> (PRE-CREATION)
>> >> - server/src/com/cloud/api/dispatch/DispatchChainFactory.java
>> >> (PRE-CREATION)
>> >> - server/src/com/cloud/api/dispatch/DispatchTask.java (PRE-CREATION)
>> >> - server/src/com/cloud/api/dispatch/DispatchWorker.java
>> >>(PRE-CREATION)
>> >> -
>> server/src/com/cloud/api/dispatch/ParamGenericValidationWorker.java
>> >> (PRE-CREATION)
>> >> - server/src/com/cloud/api/dispatch/ParamProcessWorker.java
>> >> (PRE-CREATION)
>> >> -
>> >>server/src/com/cloud/api/dispatch/ParamSemanticValidationWorker.java
>> >> (PRE-CREATION)
>> >> - server/src/com/cloud/api/dispatch/ParamUnpackWorker.java
>> >> (PRE-CREATION)
>> >> - server/src/com/cloud/network/as/AutoScaleManagerImpl.java
>> (ff2b2ea)
>> >> - server/src/com/cloud/storage/snapshot/SnapshotSchedulerImpl.java
>> >> (3159059)
>> >> - server/test/com/cloud/api/ApiDispatcherTest.java (7314a57)
>> >> - server/test/com/cloud/api/dispatch/CommandCreationWorkerTest.java
>> >> (PRE-CREATION)
>> >> - server/test/com/cloud/api/dispatch/DispatchChainFactoryTest.java
>> >> (PRE-CREATION)
>> >> -
>> >>server/test/com/cloud/api/dispatch/ParamGenericValidationWorkerTest.java
>> >> (PRE-CREATION)
>> >> - server/test/com/cloud/api/dispatch/ParamProcessWorkerTest.java
>> >> (PRE-CREATION)
>> >> -
>>
>> >>server/test/com/cloud/api/dispatch/ParamSemanticValidationWorkerTest.java
>> >> (PRE-CREATION)
>> >>
>> >> View Diff <https://reviews.apache.org/r/17888/diff/>
>> >>
>>
>>
>
