Skip to content

feat: cache generation and sequence to reduce TSM filename parsing #26798

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.

Sign up for GitHub

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

Closed
davidby-influx wants to merge 9 commits into from
Closed

feat: cache generation and sequence to reduce TSM filename parsing #26798

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
3a8a3b9
feat: cache generation and sequence to reduce TSM filename parsing
davidby-influx Sep 13, 2025
256db1b
fix: remove incorrect generation caching from DefaultPlanner.findGene…
davidby-influx Sep 14, 2025
32dbf8c
chore: format to reduce nugatory diffs
davidby-influx Sep 14, 2025
706a091
chore: add TSM generation and sequence parsing to tests
davidby-influx Sep 15, 2025
7d276df
fix: better errors and checks
davidby-influx Sep 15, 2025
40e7d75
fix: use nil check version of FileStore.ParseFileName
davidby-influx Sep 17, 2025
f71fac2
fix: option function ordering for FileStore
davidby-influx Sep 17, 2025
9dfabbb
fix: prevent possible misuse of FileStore with compaction planning
gwossum Sep 17, 2025
542214c
fix: actually sort files for adjacency testing
davidby-influx Sep 18, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions cmd/influx_tools/compact/command.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -150,6 +150,10 @@ func (sc *shardCompactor) ParseFileName(path string) (int, int, error) {
return 0, 0, errors.New("not implemented")
}

func (sc *shardCompactor) SupportsCompactionPlanning() bool {
return false
}

func newShardCompactor(path string, logger *zap.Logger) (sc *shardCompactor, err error) {
sc = &shardCompactor{
logger: logger,
Expand Down
5 changes: 5 additions & 0 deletions tsdb/engine/tsm1/compact.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -182,9 +182,14 @@ type fileStore interface {
ParseFileName(path string) (generation int, sequence int, err error)
NextGeneration() int
TSMReader(path string) (*TSMReader, error)
SupportsCompactionPlanning() bool
}

func NewDefaultPlanner(fs fileStore, writeColdDuration time.Duration) *DefaultPlanner {
if !fs.SupportsCompactionPlanning() {
// This should only happen due to developer mistakes.
panic("fileStore must support compaction planning")
Copy link
Copy Markdown

@devanbenz devanbenz Sep 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know this should only occur if a dev messes up, but, do we really want a panic in production code? Also, would it be better to have a method called MustSupportCompactionPlanning() to put this in that is similar to other parts of the codebase where we panic?

Copy link
Copy Markdown
Member

@gwossum gwossum Sep 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We had this convo in slack. Not panicking would be more invasive to the code (mainly test code), and the reason for even allowing a FileStore that didn't support compaction planning was to avoid a lot of test code changes. I'm also confident that you won't get this panic in production. If you panic in NewDefaultPlanner, influxd won't even start. You also can't a panic if the code is used properly. NewFileStore always sets a parse filename function. You have to either either to force the parse function to nil (not in codebase), create a FileStore without using NewFileStore (also not in codebase), or create various objects directly (only in tests).

Copy link
Copy Markdown
Member

@gwossum gwossum Sep 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With regards to MustSupportCompactionPlanning, SupportsCompactionPlanning isn't where the panic comes from. It leaves the choice of how to handle the issue up to the caller.

Copy link
Copy Markdown

@devanbenz devanbenz Sep 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good 👍

}
return &DefaultPlanner{
FileStore: fs,
compactFullWriteColdDuration: writeColdDuration,
Expand Down
5 changes: 5 additions & 0 deletions tsdb/engine/tsm1/compact_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4708,3 +4708,8 @@ func (w *fakeFileStore) Close() {
func (w *fakeFileStore) ParseFileName(path string) (int, int, error) {
return tsm1.DefaultParseFileName(path)
}

func (w *fakeFileStore) SupportsCompactionPlanning() bool {
// Our ParseFileName is hard-coded to always use default.
return true
}
5 changes: 5 additions & 0 deletions tsdb/engine/tsm1/file_store.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -317,6 +317,11 @@ func NewFileStore(dir string, options ...TsmReaderOption) *FileStore {
return fs
}

// SupportsCompactionPlanning returns true if f supports all functionality needed for compaction planning.
func (f *FileStore) SupportsCompactionPlanning() bool {
return f.parseFileName != nil
}

// WithObserver sets the observer for the file store.
func (f *FileStore) WithObserver(obs tsdb.FileStoreObserver) {
f.obs = obs
Expand Down