- 
                Notifications
    
You must be signed in to change notification settings  - Fork 1k
 
Enhancement: persist commit index in LogStore to accelerate recovery #613
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
base: main
Are you sure you want to change the base?
Changes from 33 commits
2e5a8a0
              ffc6b3b
              7383d96
              f6295e0
              f2ae7a9
              ce1895c
              ab50a58
              4e7e04b
              41df55e
              400a27d
              e2617e8
              6daca47
              cc09317
              fe57b32
              20e8701
              6f146e1
              a8438b0
              5e6d8a4
              e248f00
              2a913ab
              7cd6732
              92c04a0
              8e8ba07
              2020cab
              2a7d584
              bdac45b
              ed47a25
              ad87d86
              30fc43e
              e797962
              500567f
              cfffcb5
              560c0b9
              8c722fa
              300a6e7
              8d11a28
              1bdf161
              3a5d299
              File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
| 
          
            
          
           | 
    @@ -190,3 +190,28 @@ func emitLogStoreMetrics(s LogStore, prefix []string, interval time.Duration, st | |||||
| } | ||||||
| } | ||||||
| } | ||||||
| 
     | 
||||||
| type CommitTrackingLogStore interface { | ||||||
| LogStore | ||||||
| 
     | 
||||||
| // StageCommitIndex stages a new commit index to be persisted. | ||||||
| // The staged commit index MUST only be persisted in a manner that is atomic | ||||||
| // with the following StoreLogs call in the face of a crash. | ||||||
| // This allows the Raft implementation to optimize commit index updates | ||||||
| // without risking inconsistency between the commit index and the log entries. | ||||||
| // | ||||||
| // The implementation MUST NOT persist this value separately from the log entries. | ||||||
| // Instead, it should stage the value to be written atomically with the next | ||||||
| // StoreLogs call. | ||||||
| // | ||||||
| // GetCommitIndex MUST never return a value higher than the last index in the log, | ||||||
| // even if a higher value has been staged with this method. | ||||||
| // | ||||||
| // idx is the new commit index to stage. | ||||||
| StageCommitIndex(idx uint64) error | ||||||
                
      
                  dhiaayachi marked this conversation as resolved.
               
          
            Show resolved
            Hide resolved
         | 
||||||
| 
     | 
||||||
| // GetCommitIndex returns the latest persisted commit index from the latest log entry | ||||||
| // in the store at startup. | ||||||
| // It is ok to return a value higher than the last index in the log (But it should never happen). | ||||||
                
       | 
||||||
| // It is ok to return a value higher than the last index in the log (But it should never happen). | |
| // GetCommitIndex should not return a value higher than the last index in the log. If that happens, the last index in the log will be used. | 
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 we should also document here that GetCommitIndex need to return 0,nil when no commit index is found in the log.
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.
How would a GetCommitIndex() could be implemented in a real store? Would it read the latest stored log and return the commit index associated to it? What if it don't find any because those logs were stored using a store that don't support fast-recovery?
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.
For BoltDB, I imagine commit index would be a single KV in a separate bucket from logs so it would just read that and return it.
For WAL I anticipated extending the format slightly so that each commit entry in the log stores the most recently staged commit index and then re-populated that into memory when we open the log an scan it like we do with indexes.
If there is no commit index stored, we should just return 0, nil which is always safe and has the same behavior as current code I think right?
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.
If there is no commit index stored, we should just return 0, nil which is always safe and has the same behaviour as current code I think right?
I agree! I think that should be documented though. Because the API allow erroring on GetCommitIndex() it could easily be mistaken as a possible error case.
Uh oh!
There was an error while loading. Please reload this page.