Re: Review Request: Review request for bug 15372.

[Alena Prokharchyk <al...@citrix.com>]

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/5625/#review9201
-----------------------------------------------------------


The following needs to be fixed:

1) CSExceptionErrorCode class. Never define the references to class as String values because it might be broken in the future by re-packaging. Please change all these errors:

ExceptionErrorCodeMap.put("com.cloud.utils.exception.CloudRuntimeException", 4250);
ExceptionErrorCodeMap.put("com.cloud.utils.exception.ExceptionUtil", 4255);
ExceptionErrorCodeMap.put("com.cloud.utils.exception.ExecutionException", 4260);
ExceptionErrorCodeMap.put("com.cloud.utils.exception.HypervisorVersionChangedException", 4265);

This comment would be irrelevant thought once #2 is resolved. Just noting it so you would be aware of issues like that in the future.

2) I think you should refactor the way you get the exception error code in CloudRuntimeException. You use reflection, and this is very costly:


public CloudException(String message) {
        super(message);
        setCSErrorCode(CSExceptionErrorCode.getCSErrCode(this.getClass().getName()));
    }


I would do it as follows instead:

* define CloudStackException interface (or name it the way you want). Define getErrorCode() method in there, make ALL cloudStack
* define errorCode in each exception class


That would be much cleaner and proper way to do it.


Do the same for RuntimeCloudException

3) ApiDispatcher:

if (idList != null) {
+ // Iterate through entire arraylist and copy over each proxy id.
+ for (int i = 0 ; i < idList.size(); i++) {
+ IdentityProxy id = idList.get(i);
+ ex.addProxyObject(id.getTableName(), id.getValue(), id.getidFieldName());
+ s_logger.info(t.getMessage() + " db_id: " + id.getValue());
+ }


Currently you have to iterate through the list of identity proxies. Why don't you just set the Exception's with List<IdentityProxy>? It would be much cleaner, and you don't have to repeat the iteration every place in the code 

- Alena Prokharchyk


On June 28, 2012, 12:20 a.m., Venkata Siva Vijayendra Bhamidipati wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/5625/
> -----------------------------------------------------------
> 
> (Updated June 28, 2012, 12:20 a.m.)
> 
> 
> Review request for cloudstack and Alena Prokharchyk.
> 
> 
> Description
> -------
> 
> Fix for bug CS-15372 (http://bugs.cloudstack.org/browse/CS-15372).
> 
> 
> This addresses bug CS-15372.
> 
> 
> Diffs
> -----
> 
>   api/src/com/cloud/exception/CloudException.java 9ebeaad 
>   server/src/com/cloud/api/ApiDispatcher.java 0079830 
>   utils/src/com/cloud/utils/exception/RuntimeCloudException.java f2e9845 
> 
> Diff: https://reviews.apache.org/r/5625/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Venkata Siva Vijayendra Bhamidipati
> 
>