Fixes scheduler reminders app #241
Conversation
Signed-off-by: Elena Kolevska <[email protected]>
Signed-off-by: Elena Kolevska <[email protected]>
Signed-off-by: Elena Kolevska <[email protected]>
Signed-off-by: Elena Kolevska <[email protected]>
Signed-off-by: Elena Kolevska <[email protected]>
Signed-off-by: Elena Kolevska <[email protected]>
Signed-off-by: Elena Kolevska <[email protected]>
Co-authored-by: Cassie Coyle <[email protected]> Signed-off-by: Elena Kolevska <[email protected]>
| if err != nil { | ||
| log.Printf("error unmarshaling player state: %v", err) | ||
| if playerResp == nil { | ||
| log.Printf("Can't get player health.") |
There was a problem hiding this comment.
Should this be fatal?
Otherwise this will just return the go routine that is doing the bulk of this test anyways.
| log.Printf("Can't get player health.") | |
| log.Fatalf("Can't get player health.") |
There was a problem hiding this comment.
I actually think I should have just added a return in the err check above. That way, if there's a network error we'll just ignore that and retry in 5seconds. I'll do that now.
There was a problem hiding this comment.
But this will still cause the go routine to exit and not test what we want for the longhaul unless Im mistaken. Sure the app wont panic now. I think this still should fatalf on the get user bc then that negatively impacts the reminder calls for the actor? Similar to the other comment on another fatalf call.
// if they don't, the app is not testing what we need to test and we need to exit
log.Fatalf("failed to start health decay reminder: %w", err)
There was a problem hiding this comment.
Yes, sorry, that was supposed to be a continue, not a return. But anyway, I think we should decide if we're going to panic on all errors on calls to dapr, or not. My thinking was to fail on the initial ones that are a condition for the others to even be able to run, but I also see the value in failing on all, because that way we'll see in the dashboard that something's wrong.
So what do you think, should we update all of them to panic()?
There was a problem hiding this comment.
I would not panic unless you are simulating a crash in the app.
Signed-off-by: Elena Kolevska <[email protected]>
…infra into scheduler-reminders
| invokeCtx, invokeCancel := context.WithTimeout(ctx, 5*time.Second) | ||
| resp, err := client.InvokeActor(invokeCtx, req) | ||
| invokeCancel() | ||
| playerResp, err := actor.GetUser(ctx) | ||
| if err != nil { | ||
| log.Printf("error invoking actor method GetUser: %v", err) |
There was a problem hiding this comment.
| log.Printf("error invoking actor method GetUser: %v", err) | |
| log.Fatalf("error invoking actor method GetUser: %v", err) |
Signed-off-by: Elena Kolevska <[email protected]>
Addresses: #240