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

feat: Port influxd inspect verify-tsm #21615

Merged
merged 10 commits into from
Jun 9, 2021
Merged

Conversation

serenibyss
Copy link
Contributor

@serenibyss serenibyss commented Jun 4, 2021

Closes #19536

Describe your proposed changes here.
This PR ports the influxd inspect verify-tsm command from 1.x to v2. I also added a verbose flag since I thought it might be helpful since there was a lot of logging output on a database of substantial size. Additionally, there were previously no test cases, so I created some for this command, though they could probably use some work. The functional logic of the command is mostly untouched, with the work being in porting to Cobra and writing the test cases.

@serenibyss serenibyss added area/2.x OSS 2.0 related issues and PRs area/cli area/tooling and removed area/cli labels Jun 4, 2021
@danxmoran danxmoran self-requested a review June 7, 2021 15:19
cmd/influxd/inspect/tsm_verify.go Outdated Show resolved Hide resolved
cmd/influxd/inspect/tsm_verify.go Show resolved Hide resolved
cmd/influxd/inspect/tsm_verify.go Outdated Show resolved Hide resolved
cmd/influxd/inspect/tsm_verify.go Outdated Show resolved Hide resolved
cmd/influxd/inspect/tsm_verify.go Outdated Show resolved Hide resolved
cmd/influxd/inspect/tsm_verify_test.go Outdated Show resolved Hide resolved
cmd/influxd/inspect/tsm_verify_test.go Outdated Show resolved Hide resolved
cmd/influxd/inspect/tsm_verify_test.go Outdated Show resolved Hide resolved
cmd/influxd/inspect/tsm_verify_test.go Outdated Show resolved Hide resolved
cmd/influxd/inspect/tsm_verify_test.go Outdated Show resolved Hide resolved
@serenibyss serenibyss requested a review from danxmoran June 7, 2021 18:28
Copy link
Contributor

@danxmoran danxmoran left a comment

Choose a reason for hiding this comment

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

Looking good, a few more thoughts. @dgnorton would be a good person to weigh in on the test cases here to judge if they're covering the cases we expect.

cmd/influxd/inspect/tsm_verify.go Outdated Show resolved Hide resolved
cmd/influxd/inspect/tsm_verify.go Outdated Show resolved Hide resolved
cmd/influxd/inspect/tsm_verify_test.go Outdated Show resolved Hide resolved
Copy link
Contributor

@danxmoran danxmoran left a comment

Choose a reason for hiding this comment

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

The main code LGTM! I've asked @dgnorton to sanity-check the tests in case we're missing some details

@serenibyss serenibyss force-pushed the inspect-tsm-verify-dbv2 branch from 4284130 to c85b950 Compare June 8, 2021 20:35
Copy link
Contributor

@danxmoran danxmoran left a comment

Choose a reason for hiding this comment

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

Some final suggestions but LGTM overall!

cmd/influxd/inspect/tsm_verify_test.go Outdated Show resolved Hide resolved
cmd/influxd/inspect/tsm_verify_test.go Outdated Show resolved Hide resolved
@serenibyss serenibyss force-pushed the inspect-tsm-verify-dbv2 branch from 9b91a5b to a1def86 Compare June 9, 2021 15:26
@serenibyss serenibyss merged commit 8b90d23 into master Jun 9, 2021
@serenibyss serenibyss deleted the inspect-tsm-verify-dbv2 branch June 9, 2021 15:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/tooling area/2.x OSS 2.0 related issues and PRs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Port influxd inspect verify-tsm command
2 participants