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: v2 support #1439

Merged
merged 32 commits into from
Aug 29, 2016
Merged

bigtable: v2 support #1439

merged 32 commits into from
Aug 29, 2016

Conversation

callmehiphop
Copy link
Contributor

@callmehiphop callmehiphop commented Jul 18, 2016 •
edited by stephenplusplus
Loading

Fixes #1526

Moving Bigtable to v2!

TODOS:
  • New APIs
    • Instance
    • Cluster
    • grpc Operations
  • Old API Updates
    • ReadRows
  • Refactor request options for existing APIs (where applicable)
  • Tests
    • Linted
    • Unit
      • Update outdated tests
      • Add tests for new APIs
    • System
  • Documentation
    • Add docs for new APIs
    • Update old docs to reflect param changes, etc.

Docs Demo

@callmehiphop callmehiphop added don't merge api: bigtable Issues related to the Bigtable API. labels Jul 18, 2016
@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Jul 18, 2016
@coveralls
Copy link

coveralls commented Jul 18, 2016 •
edited
Loading

Coverage Status

Changes Unknown when pulling 8e633fa on callmehiphop:bigtable-v2 into * on GoogleCloudPlatform:master*.

@sduskis
Copy link
Contributor

sduskis commented Aug 17, 2016

ReadRows should get a checkbox. It's the most complicated change in v2.

@callmehiphop
Copy link
Contributor Author

/cc @lesv

@sduskis good call! I've added it (and checked it off!). I just pushed a large change that includes the new APIs and updates to the previously existing ones as well as the usage docs. All that really remains for me is a bit of testing, so whenever you get some spare time if you could take a look, that would be much appreciated!

Also @stephenplusplus if you don't mind taking a peak as well, that would be awesome :)

callmehiphop
callmehiphop reviewed Aug 17, 2016
View reviewed changes
packages/bigtable/system-test/bigtable.js
CLUSTER.setMetadata({ nodes: 4 }, done);
});

it.skip('should delete a cluster', function(done) {

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

@callmehiphop
Copy link
Contributor Author

@stephenplusplus I removed all the encode options. PTAL

stephenplusplus
stephenplusplus reviewed Aug 29, 2016
View reviewed changes
packages/bigtable/src/table.js
* table.insert(entries)
* .on('error', console.error)
* .on('finish', function() {
* // Mutations finished running.

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

stephenplusplus
stephenplusplus reviewed Aug 29, 2016
View reviewed changes
packages/bigtable/src/table.js
if (isStreamMode) {
return stream;
if (!isCallbackMode) {
['error', 'finish'].forEach(function(event) {

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

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

@stephenplusplus
Copy link
Contributor

LGTM. Is there anything waiting for me to review / comment on that I may have overlooked?

@callmehiphop
Copy link
Contributor Author

@stephenplusplus I updated a couple of system tests. I think it should be good to go if it looks good to you.

@stephenplusplus stephenplusplus merged commit 14b2f23 into googleapis:master Aug 29, 2016
@stephenplusplus
Copy link
Contributor

Excellent work, sir!

@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling fc34f2f on callmehiphop:bigtable-v2 into 069d827 on GoogleCloudPlatform:master.

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.
6 participants
@callmehiphop @coveralls @sduskis @stephenplusplus @jmdobry @googlebot