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

BigTable: Add data app profile id #5275

Closed
wants to merge 90 commits into from
Closed

BigTable: Add data app profile id #5275

wants to merge 90 commits into from

Conversation

aneepct
Copy link
Contributor

@aneepct aneepct commented May 1, 2018 •
edited
Loading

Add ability to specify the app_profile_id to use for the following Table methods:

  1. read_rows
  2. mutate_rows
  3. sample_row_keys

Luke Sneeringer and others added 30 commits January 4, 2018 10:29
Provide new auto-generated layer for Bigtable.
3edca4c
Merge branch 'master' into bigtable-regen
a677959
Add retry for read rows
d540349
Fix line lengths
4aa5702
Add parameter start_inclusive to _create_row_request
f7d4c69
Update unit test case for yield rows retry
28892ce
Fix line lengths
8d141f0
Edit table.py as requested changes
902f11b
Added code review changes from @jonparrott.
4afabb1
Fix parameter name on _RetryableReadRows class
79698b1
Add retry for Deadline Exceeded on read rows
9b778be
Fix line over-indented
cafc5ad
Fix line over-indented
f974752
Merge commit '3edca4c3299e85d4346dae49232d540a2d34002c' of https://gi…
f7242f4 
…thub.com/zakons/google-cloud-python into feature/gapic
Merge commit 'a67795951bc9f490265eda7cf511e17d2cc70921' of https://gi…
180d1d5 
…thub.com/zakons/google-cloud-python into feature/gapic
Working on gapic integration
f2e39e0
Refactore yield_rows retry
13adfd6
1. Moved last_scanned_row_key into YieldRowsData.
ca69a2f
Fix line over-indented
02c154e
Fix row_data.py
b6da7fa
Change row.py as requested
f1a2d92
Create new test case for yield_rows iterator failure
65c1c4b
Fix for start_inclusive on retry read rows.
170a173
Merge branch 'master' of https://github.com/zakons/google-cloud-python …
5da3fd3 
…into feature/read_rows_retry
Change bigtable_pb2 imports to use from gapic library.
f7a58b5
Merge branch 'feature/read_rows_retry' of https://github.com/zakons/g…
c8f664b 
…oogle-cloud-python into feature/gapic
Working on gapic
d5f0d98
Integrate bigtable GAPIC client
32df7bd
Refactoring unit tests
77ec950
Refactor to use GAPIC client
efbf2f5
@aneepct aneepct requested a review from tseaver as a code owner May 1, 2018 04:59
@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label May 1, 2018
@zakons
Copy link
Contributor

zakons commented May 1, 2018

@jonparrott @sduskis This PR is created on a fork of the GAPIC branch (PR #5178). After that PR is merged with master, we can rebase it if you prefer. Either way, please review.

Note that this is one of a series of PR's that we will create to add methods to the current Instance and Table classes to provide access to the user of the underlying, transparent GAPIC clients, specifically for management functions. All of these PR's are similarly forks of the GAPIC branch. If you have any preferences on how you want this managed, please comment. The purpose of doing multiple smaller PR's is to move these out more quickly.

@aneepct aneepct changed the title BigTable: Add app profile id on top of GAPIC integration BigTable: Add app profile id on top of GAPIC integration and truncate table May 2, 2018
@aneepct aneepct changed the title BigTable: Add app profile id on top of GAPIC integration and truncate table BigTable: Add app profile id and truncate table on top of GAPIC integration May 2, 2018
@zakons zakons mentioned this pull request May 3, 2018
Merged
@sduskis
Copy link
Contributor

sduskis commented May 3, 2018

@aneepct, can you please rebase this change on mast now that #5178 was merged?

@sduskis
Copy link
Contributor

sduskis commented May 3, 2018

@zakons, how difficult would it be to rebase to master? It's very hard to review this PR as is

@tseaver tseaver added the api: bigtable Issues related to the Bigtable API. label May 3, 2018
sduskis
sduskis suggested changes May 7, 2018
View reviewed changes
Copy link
Contributor

@sduskis sduskis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

create_app_profile does not work as is. The only parameter is the id of the profile, but the actual profile is not passed in. @zakons, can we please review this?

bigtable/google/cloud/bigtable/instance.py Outdated
@@ -239,3 +240,74 @@ def list_tables(self):
result.append(self.table(table_id))

return result

def create_app_profile(self, app_profile_id):

This comment was marked as spam.

Sign in to view

bigtable/google/cloud/bigtable/instance.py Outdated
self._client._instance_admin_client.list_app_profiles(self.name))
return list_app_profiles

def update_app_profile(self, app_profile_id, ignore_warnings=None):

This comment was marked as spam.

Sign in to view

bigtable/google/cloud/bigtable/table.py
"""

def __init__(self, table_id, instance):
def __init__(self, table_id, instance, app_profile_id=None):

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

@aneepct
Copy link
Contributor Author

aneepct commented May 9, 2018 •
edited
Loading

Split app_profile_id PR into 2 separate PR's:

#5275 use app_profile_id in data methods:

  1. read_rows
  2. mutate_rows
  3. sample_row_keys

#5315 use app_profile in admin methods:

  1. create_app_profile
  2. get_app_profile
  3. list_app_profiles
  4. update_app_profile
  5. delete_app_profile

@zakons zakons mentioned this pull request May 10, 2018
Merged
sduskis
sduskis reviewed May 18, 2018
View reviewed changes
bigtable/google/cloud/bigtable/table.py Outdated
return response_iterator

def truncate(self, timeout=60):

This comment was marked as spam.

Sign in to view

@aneepct aneepct changed the title BigTable: Add app profile id and truncate table on top of GAPIC integration BigTable: Add data app profile id on top of GAPIC integration May 22, 2018
@aneepct
Copy link
Contributor Author

aneepct commented May 22, 2018

@sduskis Separated truncate and drop by prefix to new #5360 PR

@sduskis
Copy link
Contributor

sduskis commented May 22, 2018

Can you please rebase this PR?

Also, please remove " on top of GAPIC integration" from the title.

sduskis
sduskis suggested changes May 22, 2018
View reviewed changes
Copy link
Contributor

@sduskis sduskis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

row.py needs to be updated as well. It has some data calls that require app_profile_id

@aneepct aneepct changed the title BigTable: Add data app profile id on top of GAPIC integration BigTable: Add data app profile id May 23, 2018
@aneepct
Copy link
Contributor Author

aneepct commented May 23, 2018

@sduskis I created new PR #5369 to prevent few merge conflicts going to close this PR

@aneepct aneepct closed this May 23, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sduskis sduskis sduskis requested changes

@theacodes theacodes theacodes approved these changes

@dhermes dhermes Awaiting requested review from dhermes

@lukesneeringer lukesneeringer Awaiting requested review from lukesneeringer

@tseaver tseaver Awaiting requested review from tseaver

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.
6 participants
@aneepct @zakons @sduskis @theacodes @tseaver @googlebot