[jira] [Commented] (CLOUDSTACK-6242) Potential bugs and improvements for exception handlers

["ASF subversion and git services (JIRA)" <ji...@apache.org>]

    [ https://issues.apache.org/jira/browse/CLOUDSTACK-6242?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=14051124#comment-14051124 ] 

ASF subversion and git services commented on CLOUDSTACK-6242:
-------------------------------------------------------------

Commit b2d613f53625f0dba101fa27a8a3cbdd5183740f in cloudstack's branch refs/heads/master from [~dahn]
[ https://git-wip-us.apache.org/repos/asf?p=cloudstack.git;h=b2d613f ]

CLOUDSTACK-6242: catching all to allow continuation on configuration
error (CLOUDSTACK-7039 through CLOUDSTACK-7042)

> Potential bugs and improvements for exception handlers
> ------------------------------------------------------
>
>                 Key: CLOUDSTACK-6242
>                 URL: https://issues.apache.org/jira/browse/CLOUDSTACK-6242
>             Project: CloudStack
>          Issue Type: Bug
>      Security Level: Public(Anyone can view this level - this is the default.) 
>          Components: Projects, Storage Controller
>    Affects Versions: 4.2.0
>            Reporter: Ding Yuan
>         Attachments: CLOUDSTACK-6242.patch
>
>
> Hi Cloudstack developers,
> I'm reporting a few cases where the exception handling logic could be improved. In particular, there are a few cases where the caught exception is too general (over-catch), and/or missing log messages. Attaching a patch for review. 
> There are a few cases where it looks suspicious but I do not know how to fix right now (all filenames and line numbers are based on version 4.2.1):
> =========================
> Case 1:
> Line: 375, File: "com/cloud/network/ovs/OvsTunnelManagerImpl.java"
> {noformat}
> 326:         try {
> 327:             String myIp = getGreEndpointIP(dest.getHost(), nw);
>   .. .. ..
> 373:        } catch (Exception e) {
> 374:            // I really thing we should do a better handling of these exceptions
> 375:            s_logger.warn("Ovs Tunnel network created tunnel failed", e);
> 376:        }
> {noformat}
> Comment seems to suggest for a better handling. 
> =========================
> =========================
> Case 2:
> Line: 2295, File: "com/cloud/resource/ResourceManagerImpl.java"
> {noformat}
> 2284:                 try {
> 2285:                     /*
> 2286:                      * FIXME: this is a buggy logic, check with alex. Shouldn't
> 2287:                      * return if propagation return non null
> 2288:                      */
> 2289:                     Boolean result = _clusterMgr.propagateResourceEvent(h.getId(), ResourceState.Event.UpdatePassword);
> 2290:                     if (result != null) {
> 2291:                         return result;
> 2292:                     }
> 2293:
> 2294:                     doUpdateHostPassword(h.getId());
> 2295:                 } catch (AgentUnavailableException e) {
> 2296:                 }
> {noformat}
> Seem from the comment the logic should be fixed. 
> A similar code snipeet is at:
>   Line: 2276, File: "com/cloud/resource/ResourceManagerImpl.java"
> =========================
> =========================
> Case 3:
>   Line: 184, File: "org/apache/cloudstack/api/command/user/autoscale/CreateAutoScaleVmGroupCmd.java"
> {noformat}
> 174:         try
> 175:         {
> 176:             success = _autoScaleService.configureAutoScaleVmGroup(this);
> 177:             if (success) {
> 178:                 vmGroup = _entityMgr.findById(AutoScaleVmGroup.class, getEntityId());
> 179:                 AutoScaleVmGroupResponse responseObject = _responseGenerator.createAutoScaleVmGroupResponse(vmGroup);
> 180:                 setResponseObject(responseObject);
> 181:                 responseObject.setResponseName(getCommandName());
> 182:             }
> 183:         } catch (Exception ex) {
> 184:             // TODO what will happen if Resource Layer fails in a step inbetween
> 185:             s_logger.warn("Failed to create autoscale vm group", ex);
> 186:         }
> {noformat}
> The comment, "TODO", seems to suggest for better handling. 
> =========================
> =========================
> Case 4:
>   Line: 222, File: "com/cloud/api/ApiDispatcher.java"
> This snippet is moved to ParamProcessWorker.java in the trunk.
> {noformat}
> 203:             try {
> 204:                 setFieldValue(field, cmd, paramObj, parameterAnnotation);
>      .. ..
> 220:             } catch (CloudRuntimeException cloudEx) {
> 221:                s_logger.error("CloudRuntimeException", cloudEx);
> 222:                 // FIXME: Better error message? This only happens if the API command is not executable, which typically
> 223:                 //means
> 224:                 // there was
> 225:                 // and IllegalAccessException setting one of the parameters.
> 226:                 throw new ServerApiException(ApiErrorCode.INTERNAL_ERROR, "Internal error executing API command " + cmd.getCommandName().substring(0, cmd.getCommandName().length() - 8));
> 227:             }
> {noformat}
> The "FIXME" comment seems to suggest for getter error message.
> =========================



--
This message was sent by Atlassian JIRA
(v6.2#6252)