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 > >> 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. > > >> >> 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/> >>
