Spanner: Create new instance if existing Spanner is closed by olavloite · Pull Request #5200 · googleapis/google-cloud-java

olavloite · 2019-05-18T12:39:11Z

SpannerOptions caches any Spanner instance that has been created, and hands this cached instance out to all subsequent calls to SpannerOptions.getService(). This also included closed Spanner instances. The getService() method now creates a new instance if the Spanner instance has already been closed.

sduskis · 2019-05-24T15:24:13Z

+ @Override
+ public Spanner getService() {
+ Spanner spanner = super.getService();
+ if (spanner.isClosed()) {


I think that the new object needs to be cached. Is there a way to make sure that super.getService()'s object is refreshed? The same comment applies to getRpc()

Yes, that is done by the method call ServiceOptions.createNewService() on L311 (and on L326 for getRpc()).

Where is the object cached?

It's cached in the super class. The super.getService(), which is called on first line of the method, gets the cached object from the super class. If it is closed, the createNewService() method (also defined in the super class ServiceOptions) will refresh it.
The implementation of ServiceOptions.getService() looks like this:
... privatetransientServiceTservice; ... publicServiceTgetService() { if (service == null) { service = serviceFactory.create((OptionsT) this); } returnservice; }

Won't the super class will cache the closed service, hence after super.service is closed, each new call to getService() will get a new instance?

The easiest way to show what happens is by showing the code as if it was in one class (in reality it is split between the super class and sub class, the createNewService() method is in the super class). This is what is going on:
... privatetransientServiceTservice; ... @OverridepublicSpannergetService() { Spannerspanner = super.getService(); if (spanner.isClosed()) { spanner = createNewService(); } returnspanner; } protectedServiceTcreateNewService() { service = serviceFactory.create((OptionsT) this); returnservice; }

I've added an additional test case to ensure that the correct caching behavior is actually happening here:
google-cloud-java/google-cloud-clients/google-cloud-spanner/src/test/java/com/google/cloud/spanner/SpannerOptionsTest.java
Line 89 in 75d68e7
publicvoidtestDoNotCacheClosedSpannerInstance() {

ugh... I reviewed the code a little closer, and debugged it. I see what you did now. it was a bit confusing.
Perhaps change this:
@SuppressWarnings("unchecked") public ServiceT getService() { if (service == null) { service = serviceFactory.create((OptionsT) this); } return service; } @SuppressWarnings("unchecked") protected ServiceT createNewService() { service = serviceFactory.create((OptionsT) this); return service; }
to
@SuppressWarnings("unchecked") public ServiceT getService() { if (shouldRefreshService()) { service = serviceFactory.create((OptionsT) this); } return service; } @SuppressWarnings("unchecked") protected boolean shouldRefreshService() { return service == null; }
That would make the logic a bit less spread out and readable.

That is a cleaner solution. The subclass does need access to the cached object, so I've included the cached service object to the signature of the shouldRefreshService(ServiceT) method.

ajaaym · 2019-05-24T19:04:50Z

I think the design followed here is one instance of ServiceOption refer to one instance of Service, rather than treating ServiceOption as factory for Service. If service returned from serviceOptions is used after close it would throw proper error message which would direct user to use new ServiceOptions. This change would be inconsistent with the other client in this library.

codecov · 2019-06-14T05:56:18Z

Codecov Report

Merging #5200 into master will decrease coverage by 4.09%.
The diff coverage is 0%.

@@ Coverage Diff @@## master #5200 +/- ##
============================================
- Coverage 50.82% 46.72% -4.1% - Complexity 24174 24628 +454 
============================================
Files 2271 2351 +80 Lines 229912 255993 +26081 Branches 25007 29279 +4272 ============================================
+ Hits 116845 119622 +2777 - Misses 104435 127450 +23015 - Partials 8632 8921 +289

Impacted Files	Coverage Δ	Complexity Δ
...src/main/java/com/google/cloud/ServiceOptions.java	`40.25% <0%> (-0.36%)`	`29 <4> (+2)`
...ble/data/v2/stub/EnhancedBigtableStubSettings.java	`94.66% <0%> (-3.95%)`	`17% <0%> (ø)`
...a/com/google/cloud/dialogflow/v2/AgentsClient.java	`47.72% <0%> (-2.88%)`	`19% <0%> (ø)`
.../google/cloud/dialogflow/v2beta1/AgentsClient.java	`47.72% <0%> (-2.88%)`	`19% <0%> (ø)`
...om/google/cloud/bigtable/data/v2/models/Query.java	`66.32% <0%> (-2.05%)`	`22% <0%> (-1%)`
.../v1beta1/stub/DataLabelingServiceStubSettings.java	`84.42% <0%> (-1.79%)`	`50% <0%> (+8%)`
.../cloud/dialogflow/v2beta1/stub/GrpcAgentsStub.java	`85.64% <0%> (-1.4%)`	`12% <0%> (ø)`
...oogle/cloud/dialogflow/v2/stub/GrpcAgentsStub.java	`85.64% <0%> (-1.4%)`	`12% <0%> (ø)`
...com/google/cloud/dialogflow/v2/AgentsSettings.java	`13.72% <0%> (-1.17%)`	`2% <0%> (ø)`
...oogle/cloud/dialogflow/v2beta1/AgentsSettings.java	`13.72% <0%> (-1.17%)`	`2% <0%> (ø)`
... and 107 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 32de049...1ca1c92. Read the comment docs.

SpannerOptions caches any Spanner instance that has been created, and hands this cached instance out to all subsequent calls to SpannerOptions.getService(). This also included closed Spanner instances. The getService() method now returns an error if the Spanner instance has already been closed.

SpannerOptions.getService() and SpannerOptions.getRpc() should return a new instance instead of throwing an exception if the service/rpc object has been closed.

kolea2 · 2019-06-17T14:15:41Z

+ */
+ @Override
+ public Spanner getService() {
+ // Method is only overridden in order to supply additional documentation.


My preference here would be to instead add documentation to getService in ServiceOptions so we don't have to override this (same with getRpc).

That's not possible as there are Service classes other than Spanner that are not closeable and do not have an isClosed() method, which means that the behavior that is described for Spanner is only applicable to Spanner. That is probably also the reason that the default implementation is to cache the Service object indefinitely without checking its validity.
One possible generic solution could be to change the ServiceOptions class to implement the behavior of this PR for all Service classes that implement a custom interface CloseableService. All other Service classes would keep the current default behavior.

Oh, sorry, didn't realize that was part of google-cloud-core. Can we perhaps add documentation to shouldRefreshService?

I've added documentation to the shouldRefresh... methods.
Note that we cannot/should not rely on the documentation of these methods for our users, as these methods are protected.

kolea2 · 2019-06-25T19:08:43Z

 }

+ /**
+ * @param cachedService The currently cached service object


minor - rename to cachedRpc

kolea2 · 2019-06-25T19:17:18Z

+ .setCredentials(NoCredentials.getInstance())
+ .build();
+ // Getting a service twice should give the same instance.
+ Spanner service1 = options.getService();


for completeness and coverage, can you add in a few assertions like:
assertThat(spanner1.isClosed()).isFalse();

Added assertions.

kolea2 · 2019-06-25T19:17:30Z

LGTM besides a few minor comments

chingor13 · 2019-06-28T17:26:31Z

+ * @param cachedService The currently cached service object
+ * @return true if the currently cached service object should be refreshed.
+ */
+ protected boolean shouldRefreshService(ServiceT cachedService) {


Should this and shouldRefreshRpc be static?
It looks like we're not using anything from the instance but we're allowing for that possibility in the future. If so, should we instead provide a protected getter for the cached service/rpc?

The idea is to let these two methods be overridable. The base Service and ServiceRpc classes are not closeable and therefore only does a cachedService == null check.
The Spanner class (a subclass of Service) implements AutoCloseable and SpannerOptions therefore implements a custom check in the shouldRefreshService() method that calls Spanner#isClosed() instead of the cachedService == null check.

olavloite requested a review from a teamMay 18, 2019 12:39

googlebot added the cla: yes This human has signed the Contributor License Agreement. label May 18, 2019

olavloite requested review from kolea2 and snehashah16 May 18, 2019 12:39

olavloite added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label May 21, 2019

yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label May 21, 2019

chingor13 assigned kolea2 May 22, 2019

sduskis reviewed May 24, 2019
View reviewed changes

olavloite mentioned this pull request May 27, 2019
Spanner: Do not hand out a closed Spanner instance from SpannerOptions.getService() #5187
Closed

yoshi-automation added the 🚨 critical P0 critical issue. Requires immediate fix label Jun 4, 2019

olavloite added 6 commits June 15, 2019 12:52

fix small merge error
599535f

create a new instance if the service/rpc is closed
9bbd0e2
SpannerOptions.getService() and SpannerOptions.getRpc() should return a new instance instead of throwing an exception if the service/rpc object has been closed.

add test case to ensure correct caching behavior
5561b8e

use shouldRefreshService instead of createNewService
d273479

fix merge conflicts
fb2a4db

olavloite force-pushed the spanner-create-new-instance-if-close branch from a651565 to fb2a4dbCompare June 15, 2019 11:04

kolea2 suggested changes Jun 17, 2019
View reviewed changes

olavloite added 2 commits June 17, 2019 19:04

added documentation to shouldRefresh... methods
5fbb912

removed overrides only for comments
7692138

olavloite added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jun 21, 2019

yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jun 21, 2019

kolea2 added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jun 24, 2019

yoshi-kokoro removed kokoro:force-run Add this label to force Kokoro to re-run the tests. labels Jun 24, 2019

kolea2 reviewed Jun 25, 2019
View reviewed changes

olavloite added 3 commits June 26, 2019 15:31

fixed naming
b8bf651

added assertions for isClosed()
2cc0635

fixed formatting
1ca1c92

chingor13 reviewed Jun 28, 2019
View reviewed changes

chingor13 approved these changes Jul 1, 2019
View reviewed changes

kolea2 approved these changes Jul 1, 2019
View reviewed changes

kolea2 merged commit 17cb858 into googleapis:masterJul 1, 2019

This was referenced Jul 2, 2019
Release google-cloud-java v0.99.0 #5654
Closed
Release google-cloud-java v0.99.0 #5657
Merged

Uh oh!

Conversation

olavloite commented May 18, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ajaaym commented May 24, 2019

Uh oh!

codecovBot commented Jun 14, 2019• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kolea2 commented Jun 25, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

codecovBot commented Jun 14, 2019•
edited
Loading