cloudstack-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Antonio ForniƩ Casarrubios <antonio.for...@gmail.com>
Subject Re: Review Request 17888: Dispatcher corrections, refactoring and tests. Corrects problems from previous attempt
Date Tue, 25 Feb 2014 23:47:16 GMT
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.prokharchyk@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/>
>

Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message