Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Adding basic Bigtable client. #1148

Merged
merged 1 commit into from
Sep 24, 2015
Merged

Adding basic Bigtable client. #1148

merged 1 commit into from
Sep 24, 2015

Conversation

dhermes
Copy link
Contributor

@dhermes dhermes commented Sep 21, 2015

FYI @tseaver I am essentially done with the package in https://github.com/dhermes/gcloud-python-bigtable so will now be porting over the code in (hopefully) easy to review chunks.

I'd really like to get this all in by the end of September (end of the quarter).

/cc @jgeewax

NOTE: I am holding off on bringing in documentation until there is working code in here.

@dhermes dhermes added the api: bigtable Issues related to the Bigtable API. label Sep 21, 2015
@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Sep 21, 2015
tseaver
tseaver reviewed Sep 22, 2015
View reviewed changes
gcloud/bigtable/client.py
"""

# Prevent creation of Client.connection.
_connection_class = None

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

@dhermes
Copy link
Contributor Author

dhermes commented Sep 23, 2015

@tseaver PTAL. I plan on squashing into a single commit before merging, but just added a new commit for now so you could see the diff.

Two concerns:

  • By adding :inherited-members: to the autodoc for the base client, we end up getting the factories in both the doc for Client and JSONClient. But this was the only was to liberate them from the non-public mixin
  • There is some repeated behavior since I do the credentials = get_credentials() check in the Bigtable Client constructor. If you've got ideas on how to avoid doing this twice, I'd love to DR(myself)

@dhermes
Copy link
Contributor Author

dhermes commented Sep 23, 2015

Also, I'll be teaching until 2pm Pacific tomorrow, then commuting home, but then free for the rest of the week. I'd really like to increase the velocity of these reviews so we can get the Bigtable stuff in by EOQ (it may end up being impossible, but I'd like to at least give it a chance). I've been the bottleneck on the velocity so far (ADDED: and hopefully won't be starting tomorrow afternoon).

tseaver
tseaver reviewed Sep 23, 2015
View reviewed changes
gcloud/client.py
passed falls back to the default inferred from the
environment.
If ``_connection_class`` is :data:`None`, stores the ``credentials``
on the client instead of the newly created ``connection``.

This comment was marked as spam.

Sign in to view

@tseaver
Copy link
Contributor

tseaver commented Sep 23, 2015

LGTM, modulo the note about the out-of-date docstring for gcloud.client.Client.

@dhermes
Adding basic Bigtable client.
484113d 
To do this, had to de-couple some of the existing client behavior from
convenience methods since we don't need to use the _connection_class
in Bigtable. In particular, we want both the service account
factories for the client and the implicit project behavior.
@dhermes
Copy link
Contributor Author

dhermes commented Sep 24, 2015

@tseaver I ripped out the comment and squashed the commits into a single one.

Merging now. LMK if there are issues and we can roll back.

dhermes added a commit that referenced this pull request Sep 24, 2015
@dhermes
Merge pull request #1148 from dhermes/bigtable-client
7235ab7 
Adding basic Bigtable client.
@dhermes dhermes merged commit 7235ab7 into googleapis:master Sep 24, 2015
@dhermes dhermes deleted the bigtable-client branch September 24, 2015 01:39
@dhermes dhermes mentioned this pull request Sep 30, 2015
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
api: bigtable Issues related to the Bigtable API. cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@dhermes @tseaver @googlebot