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(storage): add rpo (turbo replication) support #5003
Conversation
storage/bucket.go
Outdated
|
||
// RPODefault is used to reset RPO on an existing bucket with RPO set to RPOAsyncTurbo. | ||
// Otherwise RPODefault is equivalent to the RPO field being absent, and is always ignored | ||
RPODefault RPO = iota |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can RPO be set to default? If so, I think we'd need another unknown-type value to be the zero value. Zeros do not propagate through to the service generally. And I think we'd want to distinguish in bucket attrs between DEFAULT
vs. an unknown value (if there is a new enum addition) and/or nothing being returned for that field.
See what I have for PublicAccessPrevention, I'm assuming this should probably operate the same way.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just for some clarity, in the case of a single region bucket, RPO will not be present in the metadata. For a multiregion bucket, RPO will always come back as DEFAULT.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good note, I've changed this as suggested.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code looks good now; a couple docs comments.
Also @ddelgrosso1 , do we need an integration test for this feature?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, can merge when @ddelgrosso1 gives permission.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
Fixes #4911