fix: retention period update buckets #25989
Conversation
|
@NguyenHoangSon96 - are you ready for this PR to be reviewed? |
Hi @philjb |
There was a problem hiding this comment.
I have some concerns about making this a server-side as opposed to a frontend change. Tagging @jeffreyssmith2nd as I believe this is similar to a concern he raised.
| @@ -210,6 +210,10 @@ func (b *bucketUpdate) toInfluxDB() *influxdb.BucketUpdate { | |||
| sgd := time.Duration(*rule.ShardGroupDurationSeconds) * time.Second | |||
| upd.ShardGroupDuration = &sgd | |||
| } | |||
| } else if len(b.RetentionRules) == 0 { | |||
There was a problem hiding this comment.
I think this would have the side effect of unintentional retention period changes.
This infers from len(b.RetentionRules) == 0 that the user is attempting to update the retention period. That will not always be the case, it seems. Based on the docs for bucket updates, and the BucketUpdate struct, it looks like the bucket-update-request is not restricted to changing retention periods: the user can also update the bucket name or description. In those cases, if a user tries to update the bucket name or description, this looks like it will set their retentionPeriod and shardGroupDuration to 0 even when they are not trying to update those fields. See related concern @jeffreyssmith2nd raised here:
If OSS was updated with the same behavior, users would silently have their retention changed which could lead to disk out of space errors, etc. I think it makes more sense for the UI to explicitly send a retention rule with a seconds of 0 and shard group of the default.
IMHO, the better place to fix this is actually in the UI repository. Annoyingly, we will need a forked implementation. As @wiedld identified here, the C2 backend implementation differs in that it translates a null retentionRule to 0, and passing an explicit 0 will result in an error, unlike the OSS implementation that requires an explicit 0. Below is some suggested pseudocode for an update that is more in line with how to resolve this on the UI side, when the DELETE - NEVER button is clicked:
if (CLOUD) {
[call API without retentionRules parameter in the request (retentionRules is `null`)]
return
}
const retentionRules = [{type: 'expire' , everySeconds: 0}]
[call API with retentionRules payload - this will affect OSS only]
There was a problem hiding this comment.
@wdoconnell Thanks for looking at this, and I agree 100%. Silently updating the retention in the background isn't the behaviour we're looking for. So I think the only right place it can be fixed is in the UI.
There was a problem hiding this comment.
Hi guys
Thank you for your input. I will fix it on the front-end side.
But I have some questions. Which branch is influxdb2 cloud backend and which branch is for UI in the cloud?
|
Based on discussion, we’ve prepared another PR directly in the UI repository - influxdata/ui#7016 |
Closes influxdata/ui#1607
Fix the bug when updating the retention period of buckets:
Screen.Recording.2025-02-12.at.15.30.26.mov