-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
bigtable: implement managed backup feature #2524
Conversation
@tritone do you mind taking a look at this? |
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.
Generally looks good from the Go library perspective; I had a couple of questions. @kolea2 could you take a look as well?
|
||
// BackupIterator is an EntryIterator that iterates over log entries. | ||
type BackupIterator struct { | ||
items []*BackupInfo |
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.
I was comparing this with ProfileIterator
above, which uses the AppProfile struct from the proto directly rather than wrapping with an info struct as you do here. Is there a reason for the different approach (it seems fine to me, just curious)?
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.
Yeah, I agree there were some inconsistency, but the backup implementation was following the table APIs which use TableInfo. Probably we need a separate cleanup PR which get to a consistent naming.
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.
Ah yeah. Unfortunately we can't make breaking changes like that without releasing a new major version of the library, but it's something to keep in mind. This seems fine to me in any case.
bigtable/admin.go
Outdated
return err | ||
} | ||
|
||
// UpdateBackup updates the backup metadata in a cluster. |
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.
It looks like this only updates expireTime and not other metadata, is that correct?
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.
Yes, the backend only supports updating expireTime.
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.
I think this should be clarified in the docs, then.
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.
Added clarification in the comment.
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. Do you want to take this out of draft status so I can approve & merge?
|
||
// BackupIterator is an EntryIterator that iterates over log entries. | ||
type BackupIterator struct { | ||
items []*BackupInfo |
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.
Ah yeah. Unfortunately we can't make breaking changes like that without releasing a new major version of the library, but it's something to keep in mind. This seems fine to me in any case.
No description provided.