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

Land Bigtable v2 #1932

Merged
merged 57 commits into from
Jun 29, 2016
Merged

Land Bigtable v2 #1932

merged 57 commits into from
Jun 29, 2016

Conversation

tseaver
Copy link
Contributor

@tseaver tseaver commented Jun 29, 2016

This is the monster. See #1850 (comment) for the PRs which landed on the bigtable-v2 branch.

tseaver added 30 commits June 24, 2016 15:59
Pass the 'PROTOC_CMD' and 'GRPC_PLUGIN' values through from make's
environment to the scripts used to pick apart GRPC-specific code.
Put them in a separate '_generated_v2' subdir, to ease migration.

Attempt to factor generation process for clarity (toward #1482).
It was never actually implemented on the back-end in V1, and has been
dropped altogether in V2.
Indicate their V1 source in their names.

Prepratory to converting to V2 equivalents.
…ints

Alias Bigtable V1 imports / factories / entry point constants.
Note that {Create,Update,Delete}ColumnFamily messages all collapse to
ModifyColumnFamilies.
Update bigtable.column_family to use V2 protos
Assert that the state is 'ROW_IN_PROGRESS', and check that the completed
rows match the expected results.
Verify that completed, non-error rows match expected results after an
invalid chunk testcase.
…ror-cases

Clarify handling of V2 'ReadRowsResponse' error cases
Folds new ReadRowsResponse logic from #1907, #1915 into table row
handling.
Convert non-instance-admin protos to Bigtable V2
In V2, those operations are on the instance.
…tos.

Closes #1928.

This is a better hack for #1482, but we still really want #1384.
…1 protos.

This is a better hack for #1482, but we still really want #1384.
@daspecster
Copy link
Contributor

@tseaver, ok sounds like a plan! Thanks!

@tseaver
Copy link
Contributor Author

tseaver commented Jun 29, 2016

Just FTR, I was able to run the bigtable system tests under Python 3.4:

$ tox -re system-tests3 --notest
$ .tox/system-tests3/bin/pip install grpcio 
$ .tox/system-tests3/bin/python system_tests/run_system_test.py --package=bigtable
D0629 13:53:51.485668817   28141 ev_posix.c:101]             Using polling engine: poll
test_read_row (bigtable.TestDataAPI) ... ok
test_read_rows (bigtable.TestDataAPI) ... ok
test_read_with_label_applied (bigtable.TestDataAPI) ... ok
test_create_instance (bigtable.TestInstanceAdminAPI) ... ok
test_list_instances (bigtable.TestInstanceAdminAPI) ... ok
test_reload (bigtable.TestInstanceAdminAPI) ... ok
test_update (bigtable.TestInstanceAdminAPI) ... ok
test_create_column_family (bigtable.TestTableAdminAPI) ... ok
test_create_table (bigtable.TestTableAdminAPI) ... ok
test_delete_column_family (bigtable.TestTableAdminAPI) ... ok
test_list_tables (bigtable.TestTableAdminAPI) ... ok
test_update_column_family (bigtable.TestTableAdminAPI) ... ok
(grpcio shutdown noise elided)

----------------------------------------------------------------------
Ran 12 tests in 29.129s

OK

The bigtable-happybase system tests aren't yet Py3k-clean (they use native string literals for row keys, etc.)

@mbrukman
Copy link
Contributor

What's the status of this PR? @daspecster, are you reviewing it?

@daspecster
Copy link
Contributor

@mbrukman, I am looking it over.
I'm sure we need more eyes than mine on it :)


.. code:: python

intances = client.list_intances()

This comment was marked as spam.

This comment was marked as spam.

@lesv
Copy link

lesv commented Jun 29, 2016

@tswast PTAL

@lesv
Copy link

lesv commented Jun 29, 2016

@jonparrott PTAL - could you review this PR - ideally, we'd like to get it published today as part of Bigtable GA.

@@ -18,8 +18,8 @@

In the hierarchy of API concepts

* a :class:`Client` owns a :class:`.Cluster`
* a :class:`.Cluster` owns a :class:`Table <gcloud.bigtable.table.Table>`
* a :class:`Client` owns a :class:`.Instance`

This comment was marked as spam.

This comment was marked as spam.

@tswast
Copy link
Contributor

tswast commented Jun 29, 2016

Changes LGTM. I tested with this branch on my Hello World samples, and both ran fine. Thanks!

@tseaver
Copy link
Contributor Author

tseaver commented Jun 29, 2016

@dhermes any objections to a merge?

@lesv
Copy link

lesv commented Jun 29, 2016

@tseaver - We've had two pythonistas review. dhermes is away. It's ok to merge.

@tseaver tseaver merged commit 1a2fa6c into master Jun 29, 2016
@tseaver tseaver deleted the bigtable-v2 branch June 29, 2016 22:25
@lesv
Copy link

lesv commented Jun 29, 2016

@tseaver Can you let us know the PyPi status -- (ie submit and let us know)

@tseaver
Copy link
Contributor Author

tseaver commented Jun 29, 2016

@lesv 0.17.0 is tagged. Travis will push the release when the tag build is finished.

@lesv
Copy link

lesv commented Jun 29, 2016

@tseaver What about docs?

@tseaver
Copy link
Contributor Author

tseaver commented Jun 29, 2016

@lesv RTD already shows the 0.17.0 docs. The github.io docs get updated by the same Travis job that makes the PyPI release.

@mbrukman
Copy link
Contributor

RTD docs on instances have several misspellings: it says intances instead of instances (missing s) which I thought we addressed during code review. One of the more visible ones is right in the section header.

@mbrukman
Copy link
Contributor

mbrukman commented Jun 29, 2016

Also, RTD Bigtable usage docs talk only about clusters, does not mention instances at all.

There's also a possible typo or syntax error on the instance API page, where it says:

[...] object will be returned by create() <gcloud.bigtable.instance.Instance.create>`().

@tseaver
Copy link
Contributor Author

tseaver commented Jun 29, 2016

@mbrukman
Copy link
Contributor

Thank you, @tseaver!! Much appreciated.

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.

7 participants