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(cmd/influx): allow setting shard-group durations for buckets via API and CLI #20911

Merged
merged 20 commits into from
Mar 11, 2021

Conversation

danxmoran
Copy link
Contributor

Closes #19764

Builds off of #20620 to address review feedback and ensure shard-group duration is tracked in metadata, so the CLI can report on it in all cases.

@danxmoran danxmoran changed the title feat(cmd/influx): feat(cmd/influx): allow setting shard-group durations for buckets via API and CLI Mar 9, 2021
@danxmoran danxmoran force-pushed the dm-shard-duration-config-19764 branch from 904e57a to b7669f3 Compare March 9, 2021 22:13
CHANGELOG.md Show resolved Hide resolved
@@ -168,7 +161,7 @@ func (s *BucketClientService) UpdateBucket(ctx context.Context, id influxdb.ID,
if err != nil {
return nil, err
}
return br.toInfluxDB()
return br.toInfluxDB(), nil
Copy link
Contributor

@lesam lesam Mar 10, 2021

Choose a reason for hiding this comment

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

Is the PatchJSON call being called in the tests above? Especially for nil retention duration / shard group duration?

Copy link
Contributor Author

@danxmoran danxmoran Mar 11, 2021

Choose a reason for hiding this comment

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

Yes, in cmd/influxd/launcher/storage_test.go. The BucketService created here is a wrapper around the HTTP client. PatchJSON is called as part of the update step in each test case. All of the existing cases only set retention duration XOR shard-group duration. I need to add a case that sets both. Case updating both values is added now.

@danxmoran danxmoran requested a review from lesam March 11, 2021 15:29
},
}

for _, tt := range tests {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Lazily copy-pasted from testing/bucket_service.go

cmd/influx/bucket.go Outdated Show resolved Hide resolved
cmd/influx/bucket.go Outdated Show resolved Hide resolved
@danxmoran danxmoran requested a review from lesam March 11, 2021 16:37
Copy link
Contributor

@lesam lesam left a comment

Choose a reason for hiding this comment

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

LGTM. This feels like more change than I would have expected for piping through a single API field (especially all the stuff in http_server_bucket), which worries me a little.

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.

Add shard duration settings to the CLI
2 participants