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(bigtable): Managed Backups wrappers #57
Conversation
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.
Thanks for all of the work here! Left a few comments - also, are there any integration tests?
google/cloud/bigtable/instance.py
Outdated
self.name + "/clusters/" + cluster_id, | ||
filter_=filter_, | ||
order_by=order_by, | ||
page_size=page_size, |
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.
what is the default here if not specified?
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.
According to bigtable_table_admin.pb2.py
,
page_size:
Number of backups to be returned in the response. If 0 or
less, defaults to the server’s maximum allowed page size.
In the list_backups
method of bigtable_table_admin_client.py
, this parameter defaults to None
, so here we follow the same logic. But I agree, it should probably default to 0
rather than none-type, will change.
@kolea2 Added the integration test. To avoid repetitive LROs, everything is placed into a single comprehensive test method inside |
feat: new `__eq__` and `__ne__` convenience methods
@kolea2 PTAL. Replaced some string composition code with 'canonical' methods from |
SGTM for the |
google/cloud/bigtable/backup.py
Outdated
# See the License for the specific language governing permissions and | ||
# limitations under the License. | ||
|
||
"""A user-friendly container for Cloud Bigtable Backup.""" |
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.
not sure container is the correct word here? wrapper?. Also a Cloud Bigtable Backup
or Cloud Bigtable Backups
perhaps?
And it seems we mix google cloud bigtable
and cloud bigtable
. Maybe best to be consistent?
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.
Agreed about the wrapper
vs container
as well as Google Cloud Bigtable
. Not sure about the article though, since it implies a definition of notion here, rather than "a" (i.e. particular) Backup class. I've put it in for now, just must say that it's debatable.
google/cloud/bigtable/backup.py
Outdated
backup_id = match.group("backup_id") | ||
cluster_id = match.group("cluster_id") | ||
|
||
match = re.compile( |
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.
There is a const regex above. Should this be done similarly?
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.
Makes sense. We couldn't decide whether to declare re
as constants, hence the two ended up in different representations) It's fixed now.
""" | ||
if not self._expire_time: | ||
raise ValueError('"expire_time" parameter must be set') | ||
# TODO: Consider implementing a method that sets a default value of |
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 we create an issue to track this as a feature request?
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.
Certainly, that's the idea. I'd just wait until the initial release is merged, but please let know if you'd rather see it sooner.
:param backup_id: The ID of the Backup to be created. | ||
|
||
:type cluster_id: str | ||
:param cluster_id: (Optional) The ID of the Cluster. Required for |
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.
Seems odd we have an optional but required parameter. Can we get this by retrieving the backup? This kind of seems required? What is the use case gained by making this optional? Is this just to make it easier to create a client side object?
@mf2199 lgtm - would you be able to take a look at the build error? Edit: when the tests are fixed, please mark as ready for review (from draft) as well. Thanks! |
@kolea2 Done. According to the log, one of the errors was due to the |
A draft/beta implementation of Managed Backups' wrappers for Google Cloud Bigtable.