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, 04 Mar 2014 09:02:00 GMT
Alena, I saw it, at first I thought it would be a problem in a certain cmd
and then I saw it's the same for all of them. Actually Cmd#getCommandName()
should give us what we want here, the command name, right? Why are we
returning the cmdNameResponse instead?

On top of that, if we continue this way (getting it from the annotation as
you propose) then we would end up having N places with the same code smell
(a couple of lines getting the actual cmd name from the annotation), so
instead I should create a new method in the BaseCmd that does this, so that
these clients (like my code) only have to do: cmd.getActualCmdName()

...but then, have you got a suggestion for how to call this method
considering that we already have a method getCommandName?




2014-03-04 1:44 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/
>
> Antonio, when you log the WARN about incorrect param name, can you please past the actual
name of the command? Right now you log the response object name instead:
>
> 2014-03-03 16:41:57,806 WARN  [c.c.a.d.ParamGenericValidationWorker] (1574968208@qtp-585372613-2:ctx-0261a262
ctx-9867c49e) Received unknown parameters for command listregionsresponse. Unknown parameters
: listall
> 2014-03-03 16:41:57,843 WARN  [c.c.a.d.ParamGenericValidationWorker] (589128916@qtp-585372613-5:ctx-80f1e7ec
ctx-8d796be3) Received unknown parameters for command listprojectsresponse. Unknown parameters
: accountid
>
>
> You can get the command name from @APICommand annotation
>
>
> - Alena Prokharchyk
>
> On March 4th, 2014, 12:18 a.m. UTC, Antonio Fornie wrote:
>   Review request for cloudstack, Alena Prokharchyk, daan Hoogland, and
> Hugo Trippaers.
> By Antonio Fornie.
>
> *Updated March 4, 2014, 12:18 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
>    (b2c6734)
>    - api/src/org/apache/cloudstack/api/command/admin/user/UpdateUserCmd.java
>    (cf5d355)
>    - api/src/org/apache/cloudstack/api/command/user/autoscale/CreateAutoScaleVmProfileCmd.java
>    (570e018)
>    - server/resources/META-INF/cloudstack/core/spring-server-core-misc-context.xml
>    (fd2f5fb)
>    - server/src/com/cloud/api/ApiAsyncJobDispatcher.java (f037f2e)
>    - server/src/com/cloud/api/ApiDispatcher.java (ed95c72)
>    - server/src/com/cloud/api/ApiServer.java (25792fb)
>    - 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 (208b4a4)
>    - server/src/com/cloud/network/router/VirtualNetworkApplianceManagerImpl.java
>    (8404cab)
>    - server/src/com/cloud/storage/snapshot/SnapshotSchedulerImpl.java
>    (183a13a)
>    - 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