Add per version set timestamp for when it was last made default in queue #4524
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sushisource
approved these changes
Jun 21, 2023
| // Deletions are performed by a background process which verifies build IDs are no longer in use and safe to delete (not yet implemented). | ||
| // | ||
| // Update may fail with FailedPrecondition if it would cause exceeding the supplied limits. | ||
| func UpdateVersionSets(clock hlc.Clock, data *persistencespb.VersioningData, req *workflowservice.UpdateWorkerBuildIdCompatibilityRequest, maxSets, maxBuildIds int) (*persistencespb.VersioningData, error) { |
There was a problem hiding this comment.
Closer to the implementation
service/matching/version_sets.go
Outdated
| return nil, serviceerror.NewInvalidArgument(fmt.Sprintf("version %s already exists and is not default in set", targetedVersion)) | ||
| } | ||
| // Make the operation idempotent | ||
| return existingData, nil | ||
| return data, nil | ||
| } | ||
| return nil, serviceerror.NewInvalidArgument(fmt.Sprintf("%s requested to be made compatible with %s but both versions exist and are incompatible", targetedVersion, compatVer)) | ||
| } | ||
|
|
||
| // First duplicate the build IDs to avoid mutation |
There was a problem hiding this comment.
This comment is wrong now that it's not being cloned
| //nolint:revive // cyclomatic complexity | ||
| func updateImpl(timestamp hlc.Clock, existingData *persistencespb.VersioningData, req *workflowservice.UpdateWorkerBuildIdCompatibilityRequest) (*persistencespb.VersioningData, error) { | ||
| func updateImpl(timestamp hlc.Clock, data *persistencespb.VersioningData, req *workflowservice.UpdateWorkerBuildIdCompatibilityRequest) (*persistencespb.VersioningData, error) { |
There was a problem hiding this comment.
This should mention that it now mutates data, and possibly shouldn't return a pointer any more since that can imply it's creating a new thing
There was a problem hiding this comment.
It still needs to return a pointer because it might recreate the entire structure.
There was a problem hiding this comment.
Or data would have to be a double pointer.
bergundy
force-pushed
the
set-timestamp
branch
2 times, most recently
from
809960e
to
724af38
Compare
This was referenced Jun 21, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What changed?
馃挜 Breaking Change (for a not yet released piece of code though)
Removed the default_update_timestamp on versioning data in favor of a per version set field
queue_default_update_timestampthat denotes when it was last made default in the queue.Why?
During testing, I discovered that we may mark a previously default build ID (
vP) completely unreachable workflows immediately after a new queue default is added. This happens while there are no open workflows forvP's set and workflow tasks are currently being processed byvPworkers. When those tasks complete,vPwill be reachable again.Having a per set timestamp for when it was last made queue default will allow us to be more careful when reporting build IDs as unreachable in the reachability API and the build ID scavenger.
How did you test it?
Refactored exiting tests.
Potential risks
Need to delete all user data from existing cluster deployments for any 1.21 rc release.