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

fix: close executor when closing pool #501

Merged
merged 1 commit into from Oct 9, 2020
Merged

fix: close executor when closing pool #501

merged 1 commit into from Oct 9, 2020

Conversation

olavloite
Copy link
Contributor

@olavloite olavloite commented Oct 7, 2020

Shutdown the executor that maintains the pool when the pool is closing.

@olavloite olavloite requested a review from as a code owner Oct 7, 2020
@google-cla google-cla bot added the cla: yes label Oct 7, 2020
@olavloite olavloite requested a review from thiagotnunes Oct 7, 2020
"There is/are "
+ keysStillInUse.size()
+ " connection(s) still open. Close all connections before calling closeSpanner()");
} finally {
Copy link
Contributor Author

@olavloite olavloite Oct 7, 2020

Choose a reason for hiding this comment

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

The only real change here is the addition of the finally block, but GitHub thinks there is a lot more changed because of the changed indentation and formatting.

@olavloite
Copy link
Contributor Author

@olavloite olavloite commented Oct 7, 2020

@olavloite olavloite added the kokoro:force-run label Oct 7, 2020
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run label Oct 7, 2020
@codecov
Copy link

@codecov codecov bot commented Oct 7, 2020

Codecov Report

No coverage uploaded for pull request base (master@691a23c). Click here to learn what that means.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master     #501   +/-   ##
=========================================
  Coverage          ?   82.26%           
  Complexity        ?     2395           
=========================================
  Files             ?      138           
  Lines             ?    13414           
  Branches          ?     1240           
=========================================
  Hits              ?    11035           
  Misses            ?     1883           
  Partials          ?      496           
Impacted Files Coverage Δ Complexity Δ
...m/google/cloud/spanner/connection/SpannerPool.java 87.20% <100.00%> (ø) 31.00 <0.00> (?)

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 691a23c...87884ac. Read the comment docs.

+ " connection(s) still open. Close all connections before calling closeSpanner()");
} finally {
if (closerService != null) {
closerService.shutdown();
Copy link
Contributor

@thiagotnunes thiagotnunes Oct 7, 2020

Choose a reason for hiding this comment

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

Can this throw an exception?

Copy link
Contributor Author

@olavloite olavloite Oct 8, 2020

Choose a reason for hiding this comment

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

Normally, no. It does not declare any checked exceptions, and it also does not wait until it has actually been shutdown, so the chance that anything goes wrong is relatively low.

The method can be invoked in two ways:

  • If a client application calls SpannerPool#closeSpannerPool() to explicitly close the pool. Any exception from closerService.shutdown() would in that case be propagated to the client application. Considering the fact that it would not be a checked exception, I would say that that is a reasonable thing to do, as it might indicate a problem somewhere, for example linked to the state of the client application at that moment.
  • It is called from the shutdown hook that closes the pool automatically when the application terminates. The shutdown hook catches and ignores any errors, as any errors at that moment are (probably) not something that a client application could do anything with. The alternative would be to bubble it up, which would cause the JVM (by default) to print the error to System.err.

Copy link
Contributor

@thiagotnunes thiagotnunes Oct 9, 2020

Choose a reason for hiding this comment

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

Cool, that is fine, I was just worried that we could bubble up exceptions that we previously did not.

@product-auto-label product-auto-label bot added the api: spanner label Oct 8, 2020
@thiagotnunes thiagotnunes merged commit 2086746 into master Oct 9, 2020
19 checks passed
@thiagotnunes thiagotnunes deleted the close-executor branch Oct 9, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: spanner cla: yes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants