backup: write index full subdirs in reverse chronological order #158897
Conversation
|
This change is |
kev-cao
force-pushed
the
backup/reverse-index-subdir-order
branch
from
d559420 to
ba2fc8b
Compare
There was a problem hiding this comment.
Pull request overview
This PR implements infrastructure changes to support faster SHOW BACKUP operations by modifying how backup index subdirectories are stored. The key change is encoding subdirectory names with descending timestamps to achieve reverse chronological lexicographic ordering, which will enable more efficient backup listing operations.
Key Changes:
- Adds
DecodeDescendingTSfunction to decode descending timestamp encodings - Refactors index subdirectory naming from a simple flattened format to a composite format with both descending-encoded and human-readable timestamps
- Updates
ListSubdirsFromIndexto reverse the naturally descending-ordered results to maintain chronological order for current consumers
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/backup/backuputils/utils.go | Adds DecodeDescendingTS function to decode timestamps encoded with EncodeDescendingTS |
| pkg/backup/backupinfo/backup_index.go | Refactors index subdirectory format to use descending timestamps for reverse-chronological ordering; renames conversion functions for clarity |
| pkg/backup/backupinfo/backup_index_test.go | Removes obsolete test, fixes function name capitalization, adds comprehensive tests for index subdir conversion |
| pkg/backup/backupbase/constants.go | Updates BackupIndexDirectoryPath to include metadata directory prefix; removes obsolete BackupIndexFlattenedSubdir constant |
| pkg/backup/backupinfo/BUILD.bazel | Adds backuputils dependency for test file |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // DecodeDescendingTS decodes a time.Time encoded with EncodeDescendingTS. | ||
| func DecodeDescendingTS(encoded string) (time.Time, error) { | ||
| buffer, err := hex.DecodeString(encoded) | ||
| if err != nil { | ||
| return time.Time{}, err | ||
| } | ||
| _, tsMillis, err := encoding.DecodeUvarintDescending(buffer) | ||
| if err != nil { | ||
| return time.Time{}, err | ||
| } | ||
| return time.UnixMilli(int64(tsMillis)), nil | ||
| } |
There was a problem hiding this comment.
The new DecodeDescendingTS function lacks direct unit tests in pkg/backup/backuputils/utils_test.go. While it is indirectly tested through TestConvertIndexSubdirToSubdir, having dedicated tests would improve coverage and catch edge cases like:
- Round-trip encoding/decoding preserves the original timestamp
- Handling of timestamps at boundaries (e.g., Unix epoch, far future dates)
- Proper error handling for invalid hex strings
Consider adding a TestEncodeDecodeDescendingTS test to verify the round-trip behavior and edge cases.
kev-cao
force-pushed
the
backup/reverse-index-subdir-order
branch
from
ba2fc8b to
104591a
Compare
Pre-requisite work outlined in the faster `SHOW BACKUP` [design doc](https://docs.google.com/document/d/1_HepOulhmHcKvBrc0o7fSTJ9RXF8wkKG72W0Qgtfca8). Closes: cockroachdb#158891 Release note: None
kev-cao
force-pushed
the
backup/reverse-index-subdir-order
branch
from
104591a to
4fc3fc9
Compare
2 participants
Pre-requisite work outlined in the faster
SHOW BACKUPdesign doc.Closes: #158891
Release note: None