Skip to content
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

fix(tsdb): never use an inmem index #20313

Merged
merged 14 commits into from
Dec 23, 2020
Merged

fix(tsdb): never use an inmem index #20313

merged 14 commits into from
Dec 23, 2020

Conversation

danxmoran
Copy link
Contributor

Closes #20257

2.x was never meant to use an inmem index, but there was a fallback code path to handle cases when influxd upgrade didn't convert an inmem index into TSI. All code for the inmem index is now deleted, and the fallback is replaced by a one-time reindex operation run on shards w/o a TSI directory.

The reindex logic is inspired by the influx inspect implementation, but uses methods on Engine to read TSM / WAL data instead of listing and reading files directly from the file system.

@danxmoran
Copy link
Contributor Author

@stuartcarnie could you take a look at this one? I'm pretty confident the existing gotests cover the logic (many were failing when I still had bugs in Reindex()), but I'm not familiar enough with the code to be 100% sure.

tsdb/engine/tsm1/engine.go Show resolved Hide resolved
@danxmoran
Copy link
Contributor Author

Other note from Zoom review: try to add an integration test to check that index files are (re)generated if missing, and that query results are the same.

tsdb/engine/tsm1/engine.go Outdated Show resolved Hide resolved
if pw, ok := e.pointsWriter.(*coordinator.PointsWriter); ok {
pw.Logger = e.logger
pw.WithLogger(e.logger)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Drive-by fix for engine logging setup

Copy link
Contributor

@stuartcarnie stuartcarnie left a comment

Choose a reason for hiding this comment

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

Great work, @danxmoran – removing this will simplify future maintenance of the TSDB engine. 🥇 👏 🧹

@danxmoran danxmoran dismissed dgnorton’s stale review December 23, 2020 15:21

Requested changes addressed

@danxmoran danxmoran merged commit 9aefa6f into master Dec 23, 2020
@danxmoran danxmoran deleted the dm-no-inmem-index-20257 branch December 23, 2020 15:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

InfluxDB 2.0 sometimes uses inmem index instead of TSI
3 participants