cloudstack-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Alena Prokharchyk <Alena.Prokharc...@citrix.com>
Subject Re: Review Request 17888: Dispatcher corrections, refactoring and tests. Corrects problems from previous attempt
Date Wed, 26 Feb 2014 22:19:57 GMT
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.

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.

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.

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.


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:

* 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
* 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

Whatever one you prefer from the above, I'm fine with.

Let me know what you think,
-Alena.

From: Antonio Fornié Casarrubios <antonio.fornie@gmail.com<mailto:antonio.fornie@gmail.com>>
Date: Wednesday, February 26, 2014 at 2:02 PM
To: Alena Prokharchyk <alena.prokharchyk@citrix.com<mailto:alena.prokharchyk@citrix.com>>
Cc: "dev@cloudstack.apache.org<mailto:dev@cloudstack.apache.org>" <dev@cloudstack.apache.org<mailto:dev@cloudstack.apache.org>>,
daan Hoogland <daan.hoogland@gmail.com<mailto:daan.hoogland@gmail.com>>, Hugo
Trippaers <htrippaers@schubergphilis.com<mailto:htrippaers@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.Prokharchyk@citrix.com<mailto:Alena.Prokharchyk@citrix.com>>:
Antonio, please see my 2 comments inline.

-Alena.

On 2/26/14, 1:17 PM, "Antonio Fornié Casarrubios"
<antonio.fornie@gmail.com<mailto:antonio.fornie@gmail.com>> wrote:

>Hi Alena,
>
>I answer to your comments inline:
>
>
>2014-02-26 19:08 GMT+01:00 Alena Prokharchyk
><alena.prokharchyk@citrix.com<mailto:alena.prokharchyk@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
>>likehttp://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/>
>>



Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message