Adding Accumulator Example #355
Conversation
| } | ||
|
|
||
| log.Debug("Awaiting for " + timeout.String()) | ||
| gotSignalBeforeTimeout, _ := workflow.AwaitWithTimeout(ctx, timeout, func() bool { |
There was a problem hiding this comment.
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.
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.
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.
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.
| // 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.
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.
Unresolving, this was not changed
| if greetings.GreetingsList == nil { | ||
| greetings.GreetingsList = []AccumulateGreeting{} | ||
| } |
There was a problem hiding this comment.
| if greetings.GreetingsList == nil { | |
| greetings.GreetingsList = []AccumulateGreeting{} | |
| } |
This is not needed
|
|
||
|
|
||
|
|
There was a problem hiding this comment.
Go format usually fixes all of these extra lines, is it run?
| "fmt" | ||
| "log" | ||
| "time" | ||
|
|
There was a problem hiding this comment.
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.
| 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.
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