Worker versioning - add BuildIDs search attribute #4284
Conversation
| "HistorySizeBytes": { | ||
| "type": "long" | ||
| }, | ||
| "BuildIDs": { |
There was a problem hiding this comment.
Given that the other "ids" above are capitalized (WorkflowId, RunId), should it be better if this were BuildIds?
There was a problem hiding this comment.
Good point, I thought about that but because it's plural I think it'd be clearer if IDs stands out more.
I'm now not so convinced.
I'll get another opinion.
| currResetPoints = ms.executionInfo.AutoResetPoints.Points | ||
| } else { | ||
| currResetPoints = make([]*workflowpb.ResetPointInfo, 0, 1) | ||
| resetPoints := exeInfo.AutoResetPoints.Points |
There was a problem hiding this comment.
You're removing the check exeInfo.AutoResetPoints != nil.
Is it possible for this to be nil? If it it, this can cause a panic.
There was a problem hiding this comment.
It can't here because if we don't add a reset point the function returns early
There was a problem hiding this comment.
| resetPoints := exeInfo.AutoResetPoints.Points | |
| resetPoints := exeInfo.GetAutoResetPoints().GetPoints() |
is guaranteed to be safe.
| "HistorySizeBytes": { | ||
| "type": "long" | ||
| }, | ||
| "BuildIDs": { |
There was a problem hiding this comment.
| "BuildIDs": { | |
| "BuildIds": { |
| if !request.UseVersioning { | ||
| versionStamp = nil | ||
| } | ||
| // TODO: omit BinaryChecksum if version stamp is present and matches BinaryChecksum |
There was a problem hiding this comment.
can we/should we do anything about this? are we going to have these repeated twice in the event for now?
There was a problem hiding this comment.
Hmm... I think the worker will send both, let me track this and figure out what we want to do.
| if !stamp.GetUseVersioning() { | ||
| return taskQueue, nil | ||
| } |
There was a problem hiding this comment.
don't do this here, instead, leave the build stamp out of the AddWorkflowTaskRequest on the history side (there's no reason for matching to know about it if it's not going to use it)
There was a problem hiding this comment.
I think we'll need to to not send the stamp directly from history as I mentioned in my review of your last PR.
For now I'm leaving this on so the tests pass.
There was a problem hiding this comment.
Let's fix before we merge and this of course depends on your changes too.
tests/integrationbase.go
Outdated
| func (s *IntegrationBase) retry(attempts int, interval time.Duration, fn func() error) { | ||
| var lastErr error | ||
| for attempt := 1; attempt <= attempts; attempt++ { | ||
| lastErr = fn() | ||
| if lastErr == nil { | ||
| return | ||
| } | ||
| time.Sleep(interval) | ||
| } | ||
| s.FailNow(fmt.Sprintf("%v after %v attempts", lastErr, attempts)) | ||
| } | ||
|
|
||
| func (s *IntegrationBase) retryReasonably(fn func() error) { | ||
| s.retry(30, 500*time.Millisecond, fn) | ||
| } |
There was a problem hiding this comment.
Thanks, wasn't aware of that.
|
Ended up renaming |
Note: This commit came from a feature branch and is not expected to build.
Note: This commit came from a feature branch and is not expected to build.
Note: This commit came from a feature branch and is not expected to build.
Note: This commit came from a feature branch and is not expected to build.
Part of #3303