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.

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: Create snippets bigtable table.py #6484

Merged
merged 25 commits into from
Dec 17, 2018

Conversation

sangramql
Copy link
Contributor

Create snippets for table.py

@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Nov 13, 2018
@sangramql
Copy link
Contributor Author

@tseaver , @AVaksman Please review the snippets for table.py

@sangramql
Copy link
Contributor Author

@sduskis Please review the snippets.

bigtable/docs/snippets.py Outdated Show resolved Hide resolved
instance = client.instance(INSTANCE_ID)

table = instance.table("table_id1_samplerow")
table.create()

This comment was marked as spam.

This comment was marked as spam.

assert column_family_id in col_fams


def test_bigtable_mutations_batcher_read_rows():

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

expected_rows_data.append(row.decode('utf-8'))
expected_rows_data.append(cell_val.decode('utf-8'))

# [START bigtable_mutate_rows]

This comment was marked as spam.

This comment was marked as spam.

bigtable/docs/snippets.py Outdated Show resolved Hide resolved
table_name = table.name
# [END bigtable_table_name]

cred = os.environ.get('GOOGLE_APPLICATION_CREDENTIALS', 'project_id')

This comment was marked as spam.

This comment was marked as spam.

@sangramql sangramql changed the title Bigtable: Create snippets bigtable table.py Bigtable: Create snippets bigtable table.py (Work in progress, plz hold on further review) Nov 15, 2018
@sangramql sangramql changed the title Bigtable: Create snippets bigtable table.py (Work in progress, plz hold on further review) Bigtable: Create snippets bigtable table.py Nov 16, 2018
@sangramql
Copy link
Contributor Author

@tseaver, @AVaksman Please review the changes made according to the review.

actual_keys, offset = zip(*[(rk.row_key, rk.offset_bytes) for rk in data])
# [END bigtable_sample_row_keys]
initial_split_keys.append(b'')
assert actual_keys == initial_split_keys

This comment was marked as spam.

This comment was marked as spam.

@AVaksman
Copy link
Contributor

When I run test locally I get an error ServiceUnavailable: 503 The service is currently unavailable. by drop_by_prefix and truncates.

@sangramql
Copy link
Contributor Author

I am running on emulators, so worked fine. My rpc calls are broken so unable to test on cloud.

@AVaksman
Copy link
Contributor

After consulting with @sduskis, it looks like it might have something to do with the instance becoming replicated. As such, switching tests around seems to fix the problem. Please move test_bigtable_table_row() up right after test_bigtable_mutations_batcher() and move test_bigtable_create_additional_cluster() at the very end.

@AVaksman
Copy link
Contributor

AVaksman commented Nov 19, 2018

If this suggestion is agreed with. Please create separate instances for admin and data snippet tests rather than switching tests order.

@tseaver tseaver added the api: bigtable Issues related to the Bigtable API. label Nov 21, 2018
@sangramql
Copy link
Contributor Author

If that's the case then it looks ideal for creating separate snippets files for instance admin and table admin.

In which case, there would be separate setup and teardown. which would handle new instance for separate files.

@sangramql
Copy link
Contributor Author

sangramql commented Nov 22, 2018

@tseaver, @AVaksman, please review the changes.
As there was conflict with same instance while using the table admin and instance admin snippets, I have created separate snippets files. If you still feel we should go with single file, please suggests, I'll merge it into single file.

@crwilcox
Copy link
Contributor

Bigtable gets its noxfile from the synthtool templates. You will want to alter the last line of synth.py from

s.move(templated_files) to s.move(templated_files, excludes=['noxfile.py']) as you are adding a snippets target which isn't currently part of the template.

@sangramql
Copy link
Contributor Author

Bigtable gets its noxfile from the synthtool templates. You will want to alter the last line of synth.py from

s.move(templated_files) to s.move(templated_files, excludes=['noxfile.py']) as you are adding a snippets target which isn't currently part of the template.

@crwilcox
Done.

@sangramql
Copy link
Contributor Author

@tseaver could you please review this code?


@nox.session(python=['2.7', '3.7'])
def snippets(session):
"""Run the system test suite."""

This comment was marked as spam.

@nox.session(python=['2.7', '3.7'])
def snippets(session):
"""Run the system test suite."""
# Sanity check: Only run system tests if the environment variable is set.

This comment was marked as spam.

@tseaver
Copy link
Contributor

tseaver commented Dec 12, 2018

My two requested changes are minor. Merge when CI is green after making them.

@tseaver tseaver merged commit 8f3d5c7 into googleapis:master Dec 17, 2018
@sangramql
Copy link
Contributor Author

@sduskis Thank you for update. My apologies, I missed the notification.

@sangramql sangramql deleted the snippets_bigtable branch December 21, 2018 07:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: bigtable Issues related to the Bigtable API. cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants