Add SpannerOptions auto-configuration for emulator #2356

eddumelendez · 2020-05-06T01:40:44Z

Currently, in order to execute against the emulator, Spanner relies
on SPANNER_EMULATOR_HOST environment variable. This commit introduces
new configuration properties in order to enable and set the emulator host.

Currently, in order to execute against the emulator, Spanner relies on `SPANNER_EMULATOR_HOST` environment variable. This commit introduces new configuration properties in order to enable and set the emulator host.

codecov · 2020-05-06T01:48:53Z

Codecov Report

Merging #2356 into master will decrease coverage by 7.02%.
The diff coverage is 100.00%.

@@             Coverage Diff              @@
##             master    #2356      +/-   ##
============================================
- Coverage     81.10%   74.08%   -7.03%     
+ Complexity     2315     2102     -213     
============================================
  Files           259      260       +1     
  Lines          7563     7574      +11     
  Branches        785      785              
============================================
- Hits           6134     5611     -523     
- Misses         1103     1603     +500     
- Partials        326      360      +34

Flag	Coverage Δ	Complexity Δ
#integration	`?`	`?`
#unittests	`74.08% <100.00%> (+0.03%)`	`2102.00 <4.00> (+4.00)`

Impacted Files	Coverage Δ	Complexity Δ
...e/spanner/GcpSpannerEmulatorAutoConfiguration.java	`100.00% <100.00%> (ø)`	`2.00 <2.00> (?)`
...cp/autoconfigure/spanner/GcpSpannerProperties.java	`65.38% <100.00%> (+2.11%)`	`19.00 <2.00> (+2.00)`
...gcp/secretmanager/SecretManagerPropertySource.java	`0.00% <0.00%> (-100.00%)`	`0.00% <0.00%> (-4.00%)`
...a/spanner/repository/query/SpannerQueryMethod.java	`0.00% <0.00%> (-100.00%)`	`0.00% <0.00%> (-6.00%)`
...retmanager/SecretManagerPropertySourceLocator.java	`0.00% <0.00%> (-100.00%)`	`0.00% <0.00%> (-2.00%)`
...figure/config/GcpConfigBootstrapConfiguration.java	`0.00% <0.00%> (-100.00%)`	`0.00% <0.00%> (-2.00%)`
...epository/config/SpannerRepositoriesRegistrar.java	`0.00% <0.00%> (-100.00%)`	`0.00% <0.00%> (-3.00%)`
...ository/config/DatastoreRepositoriesRegistrar.java	`0.00% <0.00%> (-100.00%)`	`0.00% <0.00%> (-3.00%)`
...restore/GcpFirestoreEmulatorAutoConfiguration.java	`0.00% <0.00%> (-84.85%)`	`0.00% <0.00%> (-4.00%)`
...ramework/cloud/gcp/vision/DocumentOcrTemplate.java	`17.64% <0.00%> (-73.53%)`	`4.00% <0.00%> (-5.00%)`
... and 54 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7582692...6353eca. Read the comment docs.

elefeint

Thanks, Eddu, this is a really useful change for people who are starting/stopping the emulator manually. We just have to be careful to not connect to production Cloud Spanner when developers think they are starting an emulator.

elefeint · 2020-05-06T13:56:07Z

...org/springframework/cloud/gcp/autoconfigure/spanner/GcpSpannerEmulatorAutoConfiguration.java

+		SpannerOptions.Builder builder = SpannerOptions.newBuilder()
+				.setProjectId(this.properties.getProjectId())
+				.setCredentials(NoCredentials.getInstance());
+		if (this.properties.getEmulatorHost() != null) {


If we don't set SpannerOptions.emulatorHost, then the client library will connect to the real Spanner instance, which may incur costs.
I'd rather fail loudly here if emulator host is not available.

@dmitry-s @meltsufin I recall a discussion recently about using .enabled vs the property used for something. Do you remember which style was decided on?

the client library resolves the SPANNER_EMULATOR_HOST environment variable. But, I agree it can be dismiss. I think we can validate if the environment variable or the property is set, otherwise we can fail and say neither environment variable or property was set. WDYT?

Since we're providing NoCredentials, there is no risk of connecting to the real Spanner and incurring costs.

In regard to ".enabled vs the property" - .enabled is the way to go.

Yes, let's fail if "enabled" is set but "emulatorHost" is missing, since there is no way to make that configuration work. Assert.notEmpty should work.

meltsufin

It looks good overall, but can we please update the refdoc (spanner.adoc) as well?
Thanks!

meltsufin · 2020-05-06T15:09:13Z

...org/springframework/cloud/gcp/autoconfigure/spanner/GcpSpannerEmulatorAutoConfiguration.java

+		SpannerOptions.Builder builder = SpannerOptions.newBuilder()
+				.setProjectId(this.properties.getProjectId())
+				.setCredentials(NoCredentials.getInstance());
+		if (this.properties.getEmulatorHost() != null) {


Since we're providing NoCredentials, there is no risk of connecting to the real Spanner and incurring costs.

dmitry-s · 2020-05-06T15:46:49Z

...org/springframework/cloud/gcp/autoconfigure/spanner/GcpSpannerEmulatorAutoConfiguration.java

+		SpannerOptions.Builder builder = SpannerOptions.newBuilder()
+				.setProjectId(this.properties.getProjectId())
+				.setCredentials(NoCredentials.getInstance());
+		if (this.properties.getEmulatorHost() != null) {


In regard to ".enabled vs the property" - .enabled is the way to go.

eddumelendez · 2020-05-07T00:22:34Z

PR is updated with docs and also in order to keep the consistency between emulators, I renamed the previous emulator-host to host-port.

dmitry-s · 2020-05-07T10:56:22Z

docs/src/main/asciidoc/spanner.adoc

@@ -1435,10 +1437,11 @@ This command can be used to create Cloud Spanner instances:
 $ gcloud spanner instances create <instance-name> --config=emulator-config --description="<description>" --nodes=1
 ----

-Remember that you need to set the `SPANNER_EMULATOR_HOST` environment variable to use the emulator:


Not sure if we want to remove this.
Setting/unsetting the variable is a quick way to switch all your tests to using the emulator and back to normal. Might be still useful.

I'm fine with removing it. Even though it's available, it's not consistent with the other emulators. We can leave a note about it, but I'm afraid it will only confuse people.

meltsufin · 2020-05-07T13:52:59Z

docs/src/main/asciidoc/spanner.adoc

@@ -88,6 +88,8 @@ The schema for the tables will be "ON DELETE NO ACTION" if `false`. | No | `true
 | `spring.cloud.gcp.spanner.writeSessionsFraction` | Fraction of sessions to be kept prepared for write transactions | No | 0.2 - Determined by Cloud Spanner client library
 | `spring.cloud.gcp.spanner.keepAliveIntervalMinutes` | How long to keep idle sessions alive | No | 30 - Determined by Cloud Spanner client library
 | `spring.cloud.gcp.spanner.failIfPoolExhausted` |  If all sessions are in use, fail the request by throwing an exception. Otherwise, by default, block until a session becomes available. | No | `false`
+| `spring.cloud.gcp.spanner.emulator.enabled` |  Enables the usage of an emulator. If this is set to true, then you should set the `spring.cloud.gcp.spanner.host-port` to the host:port of your locally running emulator instance. | No | `false`
+| `spring.cloud.gcp.spanner.host-port` |  The host and port of the Spanner service; can be overridden to specify connecting to an already-running https://cloud.google.com/spanner/docs/emulator#installing_and_running_the_emulator[Spanner emulator] instance. | No |


@dmitry-s @elefeint Can we pick a consistent name for the emulator host property?
I see spring.cloud.gcp.datastore.host and spring.cloud.gcp.pubsub.emulator-host and spring.cloud.gcp.firestore.host-port.

meltsufin · 2020-05-07T13:55:28Z

docs/src/main/asciidoc/spanner.adoc

@@ -1435,10 +1437,11 @@ This command can be used to create Cloud Spanner instances:
 $ gcloud spanner instances create <instance-name> --config=emulator-config --description="<description>" --nodes=1
 ----

-Remember that you need to set the `SPANNER_EMULATOR_HOST` environment variable to use the emulator:


I'm fine with removing it. Even though it's available, it's not consistent with the other emulators. We can leave a note about it, but I'm afraid it will only confuse people.

...org/springframework/cloud/gcp/autoconfigure/spanner/GcpSpannerEmulatorAutoConfiguration.java

meltsufin · 2020-05-07T13:56:59Z

.../src/main/java/org/springframework/cloud/gcp/autoconfigure/spanner/GcpSpannerProperties.java

@@ -70,6 +71,8 @@
 	// Otherwise, by default, block until a session becomes available.
 	private boolean failIfPoolExhausted = false;

+	private String hostPort;


Please add a comment above like the other props.

meltsufin

@eddumelendez I reverted back to emulator-host because it's only used for the emulator. Also, since the default emulator host is localhost:9010, I made that a default.
I hope you don't mind me adding commits to your fork. :)

eddumelendez · 2020-05-07T21:40:42Z

don't mind at all. I'd prefer to use emulator.host but I think we need to align with for the other emulator properties too. WDYT?

Add SpannerOptions auto-configuration for emulator

27bfbe7

Currently, in order to execute against the emulator, Spanner relies on `SPANNER_EMULATOR_HOST` environment variable. This commit introduces new configuration properties in order to enable and set the emulator host.

elefeint suggested changes May 6, 2020

View reviewed changes

meltsufin suggested changes May 6, 2020

View reviewed changes

dmitry-s previously approved these changes May 6, 2020

View reviewed changes

eddumelendez added 2 commits May 6, 2020 18:47

Update docs

f1f8394

Rename property to host-port and add assert

6c5c52a

eddumelendez dismissed dmitry-s’s stale review via 6c5c52a May 7, 2020 00:21

dmitry-s reviewed May 7, 2020

View reviewed changes

meltsufin suggested changes May 7, 2020

View reviewed changes

eddumelendez and others added 2 commits May 7, 2020 13:36

Polish

6353eca

Rename back to emulator-host and give it a default

5b6c7de

meltsufin approved these changes May 7, 2020

View reviewed changes

elefeint approved these changes May 7, 2020

View reviewed changes

meltsufin merged commit 326cabe into spring-attic:master May 7, 2020

elefeint mentioned this pull request May 15, 2020

Test support module #2357

Draft

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add SpannerOptions auto-configuration for emulator #2356

Add SpannerOptions auto-configuration for emulator #2356

eddumelendez commented May 6, 2020

codecov bot commented May 6, 2020 •

edited

elefeint left a comment

elefeint May 6, 2020

eddumelendez May 6, 2020

meltsufin May 6, 2020

dmitry-s May 6, 2020

elefeint May 6, 2020

meltsufin left a comment

meltsufin May 6, 2020

dmitry-s May 6, 2020

eddumelendez commented May 7, 2020 •

edited

dmitry-s May 7, 2020

meltsufin May 7, 2020

meltsufin May 7, 2020

meltsufin May 7, 2020

meltsufin May 7, 2020

meltsufin left a comment

eddumelendez commented May 7, 2020

Add SpannerOptions auto-configuration for emulator #2356

Add SpannerOptions auto-configuration for emulator #2356

Conversation

eddumelendez commented May 6, 2020

codecov bot commented May 6, 2020 • edited

Codecov Report

elefeint left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

meltsufin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eddumelendez commented May 7, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

meltsufin left a comment

Choose a reason for hiding this comment

eddumelendez commented May 7, 2020

codecov bot commented May 6, 2020 •

edited

eddumelendez commented May 7, 2020 •

edited