raft: remove atomicTimestamp for fortificationtracker #138705
Conversation
|
This change is |
iskettaneh
changed the title
iskettaneh
force-pushed
the
RaftLSU3
branch
4 times, most recently
from
f9e3b74 to
29e9c09
Compare
There was a problem hiding this comment.
Reviewed 8 of 8 files at r2, all commit messages.
Reviewable status:complete! 0 of 0 LGTMs obtained (waiting on @iskettaneh)
-- commits line 7 at r2:
nit: consider adding some words about Status calls only only holding a read lock.
pkg/raft/tracker/fortificationtracker.go line 92 at r2 (raw file):
// computedLeadSupportUntil is the last computed LeadSupportUntil. We // update this value on: // [1] Every tick.
nit: "1." to match other places in the code where we use numbered lists.
pkg/raft/tracker/fortificationtracker.go line 94 at r2 (raw file):
// [1] Every tick. // [2] Every time a new fortification is recorded. // [3] On config changes. Callers of LeadSupportUntil will get this cached
nit:
"Callers of LeadSupport..." should be its own paragraph.
pkg/raft/tracker/fortificationtracker.go line 190 at r2 (raw file):
// ComputeLeadSupportUntil updates the field // computedLeadSupportUntil by computing the LeadSupportExpiration. func (ft *FortificationTracker) ComputeLeadSupportUntil(state pb.StateType) hlc.Timestamp {
Can we get rid of the return parameter here? IIUC, no one is using it.
pkg/raft/tracker/fortificationtracker.go line 199 at r2 (raw file):
return ft.computedLeadSupportUntil // fast-path for no fortification }
Should we have an early return for ft.steppingDown? That way, we don't need to do the computation.
pkg/raft/tracker/fortificationtracker.go line 359 at r2 (raw file):
// // NB: Only run by the leader. fmt.Printf("!!! IBRAHIM !!! ConfigChangeSafe. ft.leaderMaxSupported: %+v, ft.computedLeadSupportUntil: %+v\n", ft.leaderMaxSupported, ft.computedLeadSupportUntil)
nit": detritus.
pkg/raft/raft.go line 880 at r2 (raw file):
return } fmt.Printf("!!! IBRAHIM !!! sending MsgDeFortifyLeader to %v\n", to)
nit: detritus? here, and a bunch of Pritnfs in this file.
There was a problem hiding this comment.
Reviewable status:
pkg/raft/raft.go line 880 at r2 (raw file):
Previously, arulajmani (Arul Ajmani) wrote…
nit: detritus? here, and a bunch of Pritnfs in this file.
Oh sorry about that. I did some debugging yesterday night to make sure that things are working as expected, and I must have forgot to remove the debug logs.
pkg/raft/tracker/fortificationtracker.go line 190 at r2 (raw file):
Previously, arulajmani (Arul Ajmani) wrote…
Can we get rid of the return parameter here? IIUC, no one is using it.
Done.
pkg/raft/tracker/fortificationtracker.go line 199 at r2 (raw file):
Previously, arulajmani (Arul Ajmani) wrote…
Should we have an early return for
ft.steppingDown? That way, we don't need to do the computation.
Done.
There was a problem hiding this comment.
Reviewed 2 of 2 files at r3, all commit messages.
Reviewable status:
pkg/raft/tracker/fortificationtracker.go line 203 at r3 (raw file):
if ft.steppingDown { return // fast-path for stepping down
nit: this could use a better comment. Consider saying something like "We're in the process of stepping down, so we don't want to advance the LeadSupportUntil; early return"
This commit removes the struct atomicTimestamp and replaces it with hlc.Timestamp directly without locks. This is now possible as we only update LeadSupportUntil every tick, and it won't change on every status call, which are called while we hold the read lock. References: cockroachdb#137264 Release note: None
|
TFTR! bors r+ |
138705: raft: remove atomicTimestamp for fortificationtracker r=iskettaneh a=iskettaneh This commit removes the struct atomicTimestamp and replaces it with hlc.Timestamp directly without locks. This is now possible as we only update LeadSupportUntil every tick, and it won't change on every status call. References: #137264 Release note: None 138976: backup: remove support for kv.bulkio.write_metadata_sst.enabled r=jeffswenson a=jeffswenson kv.bulkio.write_metadata_sst.enabled causes backup to write out a 'metadata.sst' file. This file was never used in production, but some of the code is used by the new thin manifest format. The code that is still used was moved into new files to fix the build as a refactoring trick to ensure all unused code was deleted. Epic: none Release Note: none Co-authored-by: Ibrahim Kettaneh <[email protected]> Co-authored-by: Jeff Swenson <[email protected]>
|
This PR was included in a batch that was canceled, it will be automatically retried |
This commit removes the struct atomicTimestamp and replaces
it with hlc.Timestamp directly without locks. This is now
possible as we only update LeadSupportUntil every tick, and
it won't change on every status call.
References: #137264
Release note: None