cloudstack-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Alena Prokharchyk <Alena.Prokharc...@citrix.com>
Subject Re: Review Request 17888: Dispatcher corrections, refactoring and tests. Corrects problems from previous attempt
Date Tue, 04 Mar 2014 17:32:52 GMT
Antonio, I don¹t have a history about why we return cmdNameResponse
instead of returning actual command name. We might change the method name
in the future.

And yes, please go ahead and create a new method in BaseCmd.

-Alena.

On 3/4/14, 1:02 AM, "Antonio Fornié Casarrubios"
<antonio.fornie@gmail.com> wrote:

>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/CreateAutoScaleV
>>mProfileCmd.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.ja
>>va
>>    (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
View raw message