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

[Feature Request: Cloud Spanner] Allow unnecessary parameters to be nil when using Cloud Spanner Emulator #8134

Closed
aeroastro opened this issue Nov 12, 2020 · 7 comments · Fixed by #8416
Assignees
Labels
api: spanner Issues related to the Spanner API. type: question Request for information or clarification. Not an issue.

Comments

@aeroastro
Copy link
Contributor

Is your feature request related to a problem? Please describe.

It is often for me to use Cloud Spanner Emulator to develop app backended by Cloud Spanner.

However, current I/F of Google::Cloud::Spanner.new raises error when seemingly-unnecessary parameter is missing.

Following are the reproduction code.

require 'google/cloud/spanner'
Google::Cloud::Spanner.new(emulator_host: 'localhost:9010')

This raises the following error.

RuntimeError (Could not load the default credentials. Browse to)
https://developers.google.com/accounts/docs/application-default-credentials
for more information

Also, after running the following code with credentials argument,

require 'google/cloud/spanner'
Google::Cloud::Spanner.new(emulator_host: 'localhost:9010', credentials: 'not-existent-file-path')

it still raises error.

ArgumentError (project_id is missing)

I think the above parameters does not hold strong meaning when using emulator.
(If the above assumption is not correct, could you tell me about it?)

Describe the solution you'd like

I want the following code to be validly executed.

require 'google/cloud/spanner'
Google::Cloud::Spanner.new(emulator_host: 'localhost:9010')

I have written the conceptual design as follows.

https://github.com/googleapis/google-cloud-ruby/compare/master...aeroastro:feature/reduce-required-arguments-for-spanner-emulator?expand=1

When using this code, following code works, and we can easily understand that it connects to emulator.

require 'google/cloud/spanner'
Google::Cloud::Spanner.new(emulator_host: 'localhost:9010')
# => #<Google::Cloud::Spanner::Project:0x00007fb939155438 @service=Google::Cloud::Spanner::Service(emulator-localhost-9010), @query_options=nil>

Describe alternatives you've considered

When we specify the arguments as follows, it does not raise exception.

Google::Cloud::Spanner.new(emulator_host: 'localhost:9010', credentials: truthy_object, project_id: non_empty_object_when_stringified)

However, I would like to omit the seemingly unnecessary arguments, especially for credentials, which is overwritten by :this_channel_is_insecure in the current code.

Additional context

Also, an incompatible change (which does not affect production environments) is included in the conceptual design to reduce the internal complexity of Google::Cloud::Spanner.new.
I would like to hear some opinion on the change. If required, I can add code to maintain backward compatibility.

@aeroastro aeroastro changed the title [Feature Request: Cloud Spanner] Allow unnecessary parameters to be nil when [Feature Request: Cloud Spanner] Allow unnecessary parameters to be nil when using Cloud Spanner Emulator Nov 12, 2020
@quartzmo quartzmo added api: spanner Issues related to the Spanner API. type: question Request for information or clarification. Not an issue. labels Nov 12, 2020
@skuruppu skuruppu self-assigned this Dec 1, 2020
@skuruppu
Copy link
Contributor

skuruppu commented Dec 1, 2020

@aeroastro thanks for reporting this issue. I didn't implement the Ruby version but from my understanding of how it works in the client libraries for other languages, we're meant to set the SPANNER_EMULATOR_HOST environment variable to use the emulator as described in the docs. Then, the client library detects that the user intends to use the emulator so you don't have to set the credentials and projects.

We want the emulator usage to be consistent across all the Spanner client libraries (and across products) which is why it was implemented this way.

Does the environment variable not work in the setup you have? Or do you prefer not to use that option?

I'd like to understand the requirements before deciding the next steps.

@aeroastro
Copy link
Contributor Author

@skuruppu

Sorry for the late reply.
The background of this issue is both of them.
Especially, I think the latter is more important.

current code does not work intuitively

Following code generates an exception even when the environment variable is set.

ENV['SPANNER_EMULATOR_HOST'] = 'localhost:9010' # This is the same as `export SPANNER_EMULATOR_HOST=9010`

require 'google/cloud/spanner'
spanner = Google::Cloud::Spanner.new

The exception is as follows:

Could not load the default credentials. Browse to (RuntimeError)
https://developers.google.com/accounts/docs/application-default-credentials
for more information

This means that the environment variables are implicitly coupled in the library and the objective of the doc is not fully achieved.

So, the unnecessary project and project credentials problem exists both in argument option way and in environment variable way.

prefer to use the argument and the limitation of environment variables

First, it is pretty common for Ruby on Rails to define the database definition explicitly in the definition file, and the ruby-spanner-activerecord follows the same style.

https://guides.rubyonrails.org/active_record_multiple_databases.html#setting-up-your-application
https://github.com/googleapis/ruby-spanner-activerecord#database-connection

Especially, I would like to keep the explicit database configuration even with the SPANNER_EMULATOR_HOST as follows.
googleapis/ruby-spanner-activerecord#48 (comment)

development:
  adapter: "spanner"
  emulator_host: localhost:9010
  project: "emulator-project"  # This line can be removed by the conceptual design above
  instance: "emulator-instance" # This line can be removed by the conceptual design above
  database: "app-dev"

production:
  adapter: "spanner"
  project: "<google project name>"
  instance: "<google instance name>"
  credentials: "<google credentails file path>"
  database: "app-prod"

Also, in some environment, argument style is more suitable.

  1. Using Spanner Proxy in the production environment
    I think explicit definition and argument option is more suitable for clarity.
    https://github.com/cloudspannerecosystem/spanner-proxy

  2. Using 2 or more set of Cloud Spanner
    The environment variable SPANNER_EMULATOR_HOST can not support 2 or more set of Cloud Spanner settings.


In the conceptual design, what I would like to do is to achieve the same objective on the argument interface.

Then, the client library detects that the user intends to use the emulator so you don't have to set the credentials and projects.

@skuruppu
Copy link
Contributor

Thanks for the explanation @aeroastro. I see what you mean now.

@jiren would you be able to take the @aeroastro's design and see if it can be supported in the Ruby client?

@skuruppu skuruppu assigned jiren and unassigned skuruppu Dec 16, 2020
@jiren
Copy link
Member

jiren commented Dec 18, 2020

@aeroastro only credentials we should move inside the if emulator_host condition.
Also, project_id should be passed instead of generating for the emulator. These will helps in the seamless declaration with and without the emulator.

Still need to check with the proxy set using emulator host required credentials or not.

@aeroastro
Copy link
Contributor Author

@jiren
Thank you fore reviewing this.

Is it OK now to move on to making a Pull Request?
I am going to improve the patch on your feedback, add test, and make a Pull Request on this issue.

@jiren
Copy link
Member

jiren commented Dec 18, 2020

@jiren
Thank you fore reviewing this.

Is it OK now to move on to making a Pull Request?
I am going to improve the patch on your feedback, add test, and make a Pull Request on this issue.

Yes please create a pull request.

@aeroastro
Copy link
Contributor Author

@jiren
Thank you!
I've created a Pull Request #8416.

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. type: question Request for information or clarification. Not an issue.
Projects
None yet
4 participants