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
Add per build id timestamp for when it was last made set default #4526
Add per build id timestamp for when it was last made set default #4526
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.
Nothing blocking
QueueDefaultUpdateTimestamp: &clock, | ||
}, | ||
{ | ||
SetIds: []string{hashBuildId("2")}, | ||
BuildIds: []*persistencespb.BuildId{{Id: "2", State: persistencespb.STATE_ACTIVE, StateUpdateTimestamp: &nextClock}}, | ||
DefaultUpdateTimestamp: &nextClock, | ||
BuildIds: []*persistencespb.BuildId{{Id: "2", State: persistencespb.STATE_ACTIVE, StateUpdateTimestamp: &nextClock, SetDefaultUpdateTimestamp: &nextClock}}, |
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.
Re-using mkBuildId
would shorten things considerably
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.
Same for the rest of the other tests
@@ -863,12 +838,11 @@ func TestMergeSets(t *testing.T) { | |||
bothSetIds := mergeSetIDs([]string{hashBuildId("1")}, []string{hashBuildId("2")}) | |||
assert.Equal(t, bothSetIds, updatedData.GetVersionSets()[1].GetSetIds()) | |||
assert.Equal(t, initialData.GetVersionSets()[2].QueueDefaultUpdateTimestamp, updatedData.GetVersionSets()[1].QueueDefaultUpdateTimestamp) | |||
assert.Equal(t, nextClock, *updatedData.GetVersionSets()[1].DefaultUpdateTimestamp) | |||
assert.Equal(t, nextClock, *updatedData.VersionSets[1].BuildIds[len(updatedData.VersionSets[1].BuildIds)-1].SetDefaultUpdateTimestamp) |
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.
This access is a mouthful. Maybe make a little helper for these 3 spots
@@ -39,6 +39,10 @@ message BuildId { | |||
// (-- api-linter: core::0142::time-field-type=disabled | |||
// aip.dev/not-precedent: Using HLC instead of wall clock. --) | |||
temporal.server.api.clock.v1.HybridLogicalClock state_update_timestamp = 3; | |||
// HLC timestamp representing when this build id was last made default in its version set. |
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.
// HLC timestamp representing when this build id was last made default in its version set. | |
// HLC timestamp representing when this build id was last made default in its version set, if ever. |
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.
We currently don't expose an API to add a build id without making it set default in a set
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.
Ah, good point
// HLC timestamp representing when this build id was last made default in its version set. | ||
// (-- api-linter: core::0142::time-field-type=disabled | ||
// aip.dev/not-precedent: Using HLC instead of wall clock. --) | ||
temporal.server.api.clock.v1.HybridLogicalClock set_default_update_timestamp = 4; |
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 think became_default_timestamp
might be an easier to understand name
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.
Fair
This is a continuation of the work done in #4524.
We now store a timestamp per build id denoting when it was last made default in its set.
This information will later be used by the scavenger to ensure that we don't delete build ids that have recently been set default giving enough time for outstanding tasks to be completed and visibility index to be updated.