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