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

Spanner Beta #4028

Merged
merged 15 commits into from
Sep 26, 2017
Merged

Spanner Beta #4028

merged 15 commits into from
Sep 26, 2017

Conversation

lukesneeringer
Copy link
Contributor

@lukesneeringer lukesneeringer commented Sep 22, 2017

This shall be the "root" PR for getting Spanner to beta. Multiple PRs are going to be made using it as a base.

The remaining to-do items are:

@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Sep 22, 2017
@lukesneeringer lukesneeringer self-assigned this Sep 22, 2017
@lukesneeringer lukesneeringer changed the title Version number bump. Spanner Beta Sep 22, 2017
@lukesneeringer lukesneeringer added api: spanner Issues related to the Spanner API. spanner beta and removed api: spanner Issues related to the Spanner API. labels Sep 22, 2017
@tseaver
Copy link
Contributor

tseaver commented Sep 22, 2017

From #4027, which this PR supersedes:


Draft release notes:

google-cloud-spanner 0.27.1


Internal housekeeping not included in release notes:

@tseaver tseaver mentioned this pull request Sep 22, 2017
This was referenced Sep 22, 2017
@bjwatson
Copy link

bjwatson commented Sep 22, 2017

@lukesneeringer @jonparrott We also want a quick review of the API Reference docs to make sure it's reasonably in sync with the client library. This is just for the manual docs; the autogen docs should be fine.

@theacodes
Copy link
Contributor

@lukesneeringer is this ready for me to review the docs?

@lukesneeringer
Copy link
Contributor Author

@jonparrott Yes.

@bjwatson
Copy link

Thanks @lukesneeringer, @tseaver, and @jonparrott.

@theacodes
Copy link
Contributor

theacodes commented Sep 22, 2017

Non-documentation issues/concerns

  • google.cloud.spanner is not just an alias to google.cloud.spanner_v1. This is inconsistent with the approach we took with pubsub.

Documentation issues (these may be deferred by moving them into their own bugs):

@bjwatson
Copy link

Thanks for your detailed review @jonparrott!

@lukesneeringer is the "Non-documentation issues/concern" a breaking change to fix later. I'm assuming consistency is important here; what do you think?

@tseaver It seems like documentation items 2-6 (shown as 1-5 due to the graphic) should be fixed before the Beta release. The other presentation and organizational concerns can be fixed before GA. Do you agree @jonparrott and @lukesneeringer?

FYI @vkedia

@theacodes
Copy link
Contributor

theacodes commented Sep 22, 2017

Item 6 should really be fixed ASAP.

@bjwatson
Copy link

@jonparrott I see what you're saying with that. The Python API Reference combines manual and GAPIC at the same level, whereas the Ruby docs (and those for other languages) puts it another level below under v1.

@lukesneeringer Is this particularly difficult to fix with the current docs setup? I don't think we want to create a lot of work to finalize docs for Beta, but I do see the usability concern that Jon is raising.

@lukesneeringer
Copy link
Contributor Author

Putting in PRs for these.

@lukesneeringer
Copy link
Contributor Author

lukesneeringer commented Sep 25, 2017

Okay, I fixed all of these except the spanner alias (the item without a PR number in the comment above is waiting on #4054 to be approved, because it needs it as a baseline).

@jonparrott, I am having trouble deciding what to do about the spanner_v1 thing. I actually considered moving it, and decided against it because doing so would break people in a few cases (e.g. anyone who typed from google.cloud.spanner.client import Client, which the documentation previously recommended in one spot, now corrected).

With Pub/Sub, we were breaking everyone anyway, so it was an obvious decision. Additionally, since Pub/Sub was a thin client, it was quite necessary to do this.

I am willing to do it here also, but due to the potential customer randomization, as well as the thicker client, it is less clear to me that it is the correct path. Please advise. :-)

@theacodes
Copy link
Contributor

I think we need to make a firm decision on the api -> api_{version} going forwards - either it's a direct alias or it adds functionality. Either spanner or pubsub will need to change. I strongly think we should follow what pubsub did - spanner is transitioning into beta so breaking is allowed (although less than ideal, sure).

@theacodes
Copy link
Contributor

fwiw, the pubsub samples use google.cloud.pubsub_v1 explicitly.

@lukesneeringer
Copy link
Contributor Author

Okay, I am happy to change it then. :-)

@bjwatson
Copy link

Thanks, @lukesneeringer for fixing just about everything. I agree that I'm happier with all the API suffixes in a post-4054 world, too.

What's the PR for the alias fix?

@lukesneeringer
Copy link
Contributor Author

What's the PR for the alias fix?

There is not one yet. I had wall-to-wall meetings followed by lunch. :-)

@bjwatson
Copy link

Understood. Must be Monday. Lots of meetings.

@lukesneeringer
Copy link
Contributor Author

@jonparrott @bjwatson Okay, #4064 exists.

This is probably a breaking change at the margins. In other words, most users will not be affected, but some might be. In particular, we did have at least one doc that told users to from google.cloud.spanner.client import Client, and that will break.

Do we need any kind of migration guide?

@bjwatson
Copy link

@lukesneeringer We should at least have release notes that explain this breaking change.

@lukesneeringer
Copy link
Contributor Author

@bjwatson We will have release notes either way. :-)

@lukesneeringer lukesneeringer merged commit 716c2f9 into master Sep 26, 2017
@lukesneeringer lukesneeringer deleted the spanner-beta branch September 26, 2017 20:44
@bjwatson
Copy link

Woo hoo! Great job @lukesneeringer, @tseaver, and @jonparrott!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: spanner Issues related to the Spanner API. cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants