[GitHub] [incubator-yunikorn-k8shim] wilfred-s commented on a change in pull request #377: [YUNIKORN-1102] shim context getTask error is ignored

[GitBox <gi...@apache.org>]

wilfred-s commented on a change in pull request #377:
URL: https://github.com/apache/incubator-yunikorn-k8shim/pull/377#discussion_r821250941



##########
File path: pkg/cache/context.go
##########
@@ -700,17 +700,24 @@ func (ctx *Context) RemoveTask(appID, taskID string) error {
 	return fmt.Errorf("application %s is not found in the context", appID)
 }
 
-func (ctx *Context) getTask(appID string, taskID string) (*Task, error) {
+func (ctx *Context) getTask(appID string, taskID string) *Task {
 	ctx.lock.RLock()
 	defer ctx.lock.RUnlock()
-	if app, ok := ctx.applications[appID]; ok {
+	if app := ctx.GetApplication(appID); app != nil {

Review comment:
       Looking at what we log in case of an issue I think we have a flow issue. If the task conversion fails we log 3 things: conversion failed, task not found and app not found. Really confusing. Go coding guidelines also state to prefer to return early and do not indent the correct case, indent the error handling
   That would get a better user experience would be to just log the most specific message.
   ```
   app := ctx.GetApplication(appID)
   if app == nil {
       log.Logger().....
       return nil
   }
   managedTask, err := app.GetTask(taskID)
   if err != nil {
       log.Logger().....
       return nil
   }
   task, valid := managedTask.(*Task)
   if !valid {
       log.Logger().....
       return nil
   }
   return task
   ```




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: reviews-unsubscribe@yunikorn.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org