-
Notifications
You must be signed in to change notification settings - Fork 202
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Adding Accumulator Example #355
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's still a lot of little stuff, but I did a first skim for obvious things
} | ||
|
||
log.Debug("Awaiting for " + timeout.String()) | ||
gotSignalBeforeTimeout, _ := workflow.AwaitWithTimeout(ctx, timeout, func() bool { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
May be worth noting that you're adding two events for every task that has a signal here. You are adding a timer start and potentially a timer cancel. This is technically fine, but may be worth noting. Some users choose to try to reuse timers instead of creating a new one each time.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I reworked this due to your feedback above about using a timer with a selector.
@Quinn-With-Two-Ns - unsure if you want to give this a review |
@cretz I saw this, I just had no additional comments to add after yours |
…ching signals at the end
"go.temporal.io/sdk/workflow" | ||
) | ||
|
||
/** |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably don't need to duplicate readme content in the file
if totalLeft <= 0 { | ||
return 0, nil | ||
} | ||
if signalToSignalTimeout > totalLeft { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't you mean:
if signalToSignalTimeout > totalLeft { | |
if totalLeft > signalToSignalTimeout { |
Since signalToSignalTimeout
says it's the "maximum"?
* signal with the GetNextTimeout() function. | ||
*/ | ||
|
||
// signalToSignalTimeout is them maximum time between signals |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// signalToSignalTimeout is them maximum time between signals | |
// signalToSignalTimeout is the maximum time between signals |
// This waits for the greater of the remaining fromStartTimeout and signalToSignalTimeout | ||
// fromStartTimeout and signalToSignalTimeout can be adjusted to wait for the right amount of time as desired | ||
// This resets with Continue As New | ||
func (a *AccumulateGreeting) GetNextTimeout(ctx workflow.Context, startTime time.Time, exitRequested bool) (time.Duration, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this a method on AccumulateGreeting
struct? And why exported?
|
||
|
||
// GetNextTimeout returns the maximum time allowed to wait for the next signal. | ||
func (a *AccumulateGreeting) GetNextTimeout(ctx workflow.Context, timeToExit bool, firstSignalTime time.Time ) (time.Duration, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unresolving, this was not changed
if greetings.GreetingsList == nil { | ||
greetings.GreetingsList = []AccumulateGreeting{} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if greetings.GreetingsList == nil { | |
greetings.GreetingsList = []AccumulateGreeting{} | |
} |
This is not needed
|
||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Go format usually fixes all of these extra lines, is it run?
"fmt" | ||
"log" | ||
"time" | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Usually stdlib imports are grouped together and modern autoformat fixes this usually
|
||
"math/rand" | ||
|
||
accumulator "github.com/temporalio/samples-go/accumulator" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
accumulator "github.com/temporalio/samples-go/accumulator" | |
"github.com/temporalio/samples-go/accumulator" |
No need to alias
} | ||
defer c.Close() | ||
|
||
// setup which tests to run |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are inconsistent with whether you capitalize/punctuate sentences. Also, this is a bit confusing to follow.
What was changed
Adding accumulator pattern sample: accumulator and associated readme changes
Why?
This is a pattern that's pretty common, and I wanted to have a sample that I could share about it. Gathering a lot of signals, managing continue as new, demonstrating signal with start, all good stuff to have.
Checklist
Closes N/A
How was this tested:
Tested locally, tests in accumulate_signals_workflow_test.go
Any docs updates needed?
No