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

spanner: remove appengine-specific numChannels. #2513

Merged
merged 2 commits into from
Jul 9, 2020
Merged

spanner: remove appengine-specific numChannels. #2513

merged 2 commits into from
Jul 9, 2020

Conversation

hengfengli
Copy link
Contributor

Fixes: #1553

@hengfengli hengfengli added the api: spanner Issues related to the Spanner API. label Jun 25, 2020
@hengfengli hengfengli self-assigned this Jun 25, 2020
@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Jun 25, 2020
olavloite
olavloite approved these changes Jun 25, 2020
View reviewed changes
Copy link
Contributor

@olavloite olavloite left a comment

Choose a reason for hiding this comment

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

The change itself looks good to me. What is the exact reason that we can drop this now?

@hengfengli
Copy link
Contributor Author

The change itself looks good to me. What is the exact reason that we can drop this now?

From the issue description, it seems that we somehow must do this for Go 1.9 because App Engine used to support that. Now, the standard env only supports 1.12+ and 1.11 so that we don't have to do this. But from what I can see, the flexible env still supports 1.9 (https://cloud.google.com/appengine/docs/go).

@hengfengli
Copy link
Contributor Author

In https://github.com/googleapis/google-cloud-go#go-versions-supported, we say:

We support the two most recent major versions of Go. If Google App Engine uses an older version, we support that as well.

Now, I am a bit confused. In the flexible env of Google App Engine, it still supports 1.9 as its oldest version. @skuruppu Do you think we should merge this? This issue has been in the queue for almost 1 year.

@hengfengli
Copy link
Contributor Author

@broady Do you have any thought about this? Are we safe to remove them?

@skuruppu skuruppu requested a review from broady June 29, 2020 22:59
@hengfengli
Copy link
Contributor Author

@broady Just a follow-up with this PR. Do you have any thoughts?

@broady
Copy link
Contributor

broady commented Jul 8, 2020

Sorry, didn't see this. LGTM.

@hengfengli hengfengli merged commit d7e16e7 into googleapis:master Jul 9, 2020
@hengfengli hengfengli deleted the remove-appengine-specific-numchannels branch July 9, 2020 00:27
@Capstan
Copy link

Capstan commented Jul 9, 2020

In terms of safety, https://cloud.google.com/appengine/docs/deprecations/ states Go 1.9 was shut down June 30, 2020, so there's not a concern there.

Also 🎉 !

@apstndb apstndb mentioned this pull request Aug 24, 2020
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@olavloite olavloite olavloite approved these changes

@skuruppu skuruppu Awaiting requested review from skuruppu

@broady broady Awaiting requested review from broady

Assignees

@hengfengli hengfengli

Labels
api: spanner Issues related to the Spanner 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.

spanner: Remove AppEngine-specific numChannels behavior
5 participants
@hengfengli @broady @Capstan @olavloite @googlebot