check wf status to avoid infinite loops #45
Conversation
|
@realstraw please review it. |
|
@damianxu88 @schowsf Please review. |
orchard/orchard_client.go
Outdated
| return false, err | ||
| } | ||
| if resp.StatusCode != http.StatusOK { | ||
| return false, fmt.Errorf("workflow %s is not found", orchardID) |
There was a problem hiding this comment.
not found should be 404, probably not a good idea to treat all non 200 response as 404
| resp, err := c.Details(orchardID) | ||
| if err != nil { | ||
| return false, err | ||
| } |
There was a problem hiding this comment.
make sure you close the body afterwards with defer rsp.Body.Close()
orchard/orchard_client.go
Outdated
| @@ -86,6 +86,33 @@ func (c OrchardRestClient) create(result string) (string, error) { | |||
| return string(orchardID), nil | |||
| } | |||
|
|
|||
| func (c OrchardRestClient) IsActivated(orchardID string) (bool, error) { | |||
There was a problem hiding this comment.
Let's make it to Details and return parsed details instead. Whoever is calling this should deal with what status it returns
service/scheduler.go
Outdated
| @@ -158,6 +158,10 @@ func (s *Scheduler) activateWorkflow( | |||
| wf table.Workflow, | |||
| ) string { | |||
| if err := client.Activate(swf.OrchardID); err != nil { | |||
| isActivated, err2 := client.IsActivated(swf.OrchardID) | |||
There was a problem hiding this comment.
where is the infinite loop coming from? it's activated but sprinkler think it's not?
There was a problem hiding this comment.
Yes, when a wf is activated but sprinkler thinks it's not and keeps activating the wf in every second.
service/scheduler.go
Outdated
| @@ -158,6 +158,10 @@ func (s *Scheduler) activateWorkflow( | |||
| wf table.Workflow, | |||
| ) string { | |||
| if err := client.Activate(swf.OrchardID); err != nil { | |||
| details, err2 := client.Details(swf.OrchardID) | |||
| if err2 == nil && details.Status != "pending" { | |||
There was a problem hiding this comment.
could details be nil? does it need a nil check before details.Status
There was a problem hiding this comment.
Maybe we should just change client.Activate to check the return code upon error and send a Detail call, instead of doing it here.
There was a problem hiding this comment.
could
detailsbe nil? does it need a nil check beforedetails.Status
@schowsf yeah, can add a nil check before details.Status
No description provided.