bigtable: implement managed backup feature #2524
Conversation
googlebot
added
the
cla: yes
|
@tritone do you mind taking a look at this? |
|
|
||
| // BackupIterator is an EntryIterator that iterates over log entries. | ||
| type BackupIterator struct { | ||
| items []*BackupInfo |
There was a problem hiding this comment.
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.
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.
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.
It looks like this only updates expireTime and not other metadata, is that correct?
There was a problem hiding this comment.
Yes, the backend only supports updating expireTime.
There was a problem hiding this comment.
I think this should be clarified in the docs, then.
There was a problem hiding this comment.
Added clarification in the comment.
|
|
||
| // BackupIterator is an EntryIterator that iterates over log entries. | ||
| type BackupIterator struct { | ||
| items []*BackupInfo |
There was a problem hiding this comment.
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.