-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/17888/#review35542
-----------------------------------------------------------
Antonio, in general looks good to me. There are some minor fixes that need to
be done though.
1) ApiServer.java
@Inject()
177
protected DispatchChainFactory dispatchChainFactory = null;
Why do you use @Inject with ()?
2) ApiServlet.java
domainIdArr = (String[])params.get(ApiConstants.DOMAIN__ID);
* Why do we have DOMAIN__ID and DOMAIN_ID as separate parameters?
public static final String DOMAIN_ID = "domainid";
public static final String DOMAIN__ID = "domainId";
The above doesn't look right to me. Why do we care about the case? the API
parameter is always transformed to all lowercase letters
3) CommandCreationWorker.java
throw new ServerApiException(ApiErrorCode.INTERNAL_ERROR,
50
"Trying to invoke creation on a Command that is not
BaseAsyncCreateCmd");
* Don't hardcode the class name; retrieve it from the .simpleName method called
on the class object
4) DispatchChain
for (final DispatchWorker worker : workers) {
if (!worker.handle(task)) {
break;
}
}
* Why do we just break when workder can't handle the task? Aren't we supposed
to do something?
* Can you please add more logging? At least log in trace that some worker
handled/coudln't handle the task
4) I can see that you do a lot of initialization like that for private
variables:
protected AccountManager _accountMgr = null;
its not necessary step as private variables are being initialized to NULL by
default during declaration. Can you please clean it out?
- Alena Prokharchyk
On Feb. 26, 2014, 9:12 a.m., Antonio Fornie wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/17888/
> -----------------------------------------------------------
>
> (Updated Feb. 26, 2014, 9:12 a.m.)
>
>
> Review request for cloudstack, Alena Prokharchyk, daan Hoogland, and Hugo
> Trippaers.
>
>
> 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.
>
>
> 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
> 592b828
> api/src/org/apache/cloudstack/api/command/admin/user/UpdateUserCmd.java
> de6e550
>
> 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
>
> Diff: https://reviews.apache.org/r/17888/diff/
>
>
> 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 like
> http://localhost:8096/client/api?command=createSecondaryStagingStore&url=httpbla&details[0].key=region&details[0].value=canada&details[1].key=element&details[1].value=fire
>
>
> Thanks,
>
> Antonio Fornie
>
>
