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