Hi Alena, Thanks for the review and comments.
We expect two kinds of parameters: 1. Generic parameters as you mention. I include several already (command, sessionKey, domain...), but I assumed I could be missing some. I will add the ones you mention and I will appreciate if other people can also let me know if I missed any others (or add them directly to ParamGenericValidationWorker class). 2. Specific parameters for the current command, which already include the inherited parameters, actually I just reused the method that gets the parameters from any class between BaseCmd and the current command class in order to set the values. But yes, I saw that listAll was there very often just to realize that in many List commands the caller sends it althought it is completely ignored. This is because the listAll parameter is inherited from classes (like BaseListDomainResourcesCmd) that not all the List Cmd classes extend. I also thought for a moment that listAll would be there and actually I am not sure if we should remove the parameter from the command request or include it in these commands too and actually use it. Any other parameters are considered unknown. About the logs, you are right, DEBUG should be used instead of WARN when nothing bad is happening. But I think this is a WARN. A request should never have unexpected parameters. In most implementations for remote calls (ie: typical SOAP implementations) adding unkown params would fail from the beginning upon request validation. But after seeing so many cases of unknown/ignored params and to keep retrocompatibility we decided to just log it, and chose WARN instead of ERROR because it's actually not causing any harm. Still we should WARN that the client (like the web ui) is assuming the listAll param will have some effect, and it won't. The listAll param is a good example of it and the WARN will remind us to fix it. In the case of listAll, do you propose to remove it from the request or to change the server side implementation? Thanks again. Cheers Antonio 2014-02-25 22:22 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/ > > Also please change the level of the log from WARN to DEBUG. When incorrect > parameter is passed in, there is no harm as its basically ignored at the end. > WARN is used when something harmful is going on. > > > - Alena Prokharchyk > > On February 25th, 2014, 12:11 p.m. UTC, Antonio Fornie wrote: > Review request for cloudstack, Alena Prokharchyk, daan Hoogland, and > Hugo Trippaers. > By Antonio Fornie. > > *Updated Feb. 25, 2014, 12:11 p.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/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/> >
