Antonio, please see inline. On 2/25/14, 3:47 PM, "Antonio Fornié Casarrubios" <antonio.for...@gmail.com> wrote:
>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). Check ApiServer/ApiServlet to check what parameters can be expected in addition to ones defined on the command level > >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. Got 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? I think our UI does append listAll=true to all the list requests done from the admin UI, as admin is supposed to see all the resources in the cloud. And it was easier for UI folks to just add the parameter to each list call in generic way. I think by now it would be easier if you change your checker implementation to ignore listAll append for the list commands. > >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/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/> >>
