Fix: multiple calls to end of span #75

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

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.

mayurkale22 · 2020-02-14T17:25:50Z

@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://github.com/googleapis/google-cloud-java/pull/2677/files#r157323309 for original reason.

skuruppu

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

mayurkale22 · 2020-02-19T21:04:54Z

@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 · 2020-02-20T10:22:32Z

@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://github.com/mayurkale22/java-spanner/pull/2 should do the trick.

googlebot · 2020-02-20T17:28:29Z

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 · 2020-02-20T18:06:43Z

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.

googlebot · 2020-02-20T22:50:41Z

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.

googlebot · 2020-02-20T22:52:38Z

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 · 2020-02-21T00:50:02Z

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

olavloite

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

googlebot · 2020-02-21T22:42:51Z

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.

mayurkale22 · 2020-02-21T22:43:36Z

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.

mayurkale22 · 2020-02-21T22:44:47Z

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

mayurkale22 · 2020-02-25T17:30:57Z

@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://togithub.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://togithub.com/googleapis/java-spanner/blob/master/CHANGELOG.md#1510httpswwwgithubcomgoogleapisjava-spannercomparev1500v1510-2020-03-13) [Compare Source](https://togithub.com/googleapis/java-spanner/compare/v1.50.0...v1.51.0) ##### Features - add backend query options ([#90](https://www.github.com/googleapis/java-spanner/issues/90)) ([e96e172](https://www.github.com/googleapis/java-spanner/commit/e96e17246bee9691171b46857806d03d1f8e19b4)) - add QueryOptions proto ([#84](https://www.github.com/googleapis/java-spanner/issues/84)) ([eb8fc37](https://www.github.com/googleapis/java-spanner/commit/eb8fc375bbd766f25966aa565e266ed972bbe818)) ##### Bug Fixes - never use credentials in combination with plain text ([#98](https://www.github.com/googleapis/java-spanner/issues/98)) ([7eb8d49](https://www.github.com/googleapis/java-spanner/commit/7eb8d49cd6c35d7f757cb89009ad16be601b77c3)) ##### Dependencies - update dependency com.google.cloud:google-cloud-core-bom to v1.93.1 ([#91](https://www.github.com/googleapis/java-spanner/issues/91)) ([29d8db8](https://www.github.com/googleapis/java-spanner/commit/29d8db8cfc9d12824b9264d0fb870049a58a9a03)) - update dependency io.opencensus:opencensus-api to v0.25.0 ([#95](https://www.github.com/googleapis/java-spanner/issues/95)) ([57f5fd0](https://www.github.com/googleapis/java-spanner/commit/57f5fd0f3bee4b437f48b6a08ab3174f035c8cca)) ### [`v1.50.0`](https://togithub.com/googleapis/java-spanner/blob/master/CHANGELOG.md#1510httpswwwgithubcomgoogleapisjava-spannercomparev1500v1510-2020-03-13) [Compare Source](https://togithub.com/googleapis/java-spanner/compare/v1.49.2...v1.50.0) ##### Features - add backend query options ([#90](https://www.github.com/googleapis/java-spanner/issues/90)) ([e96e172](https://www.github.com/googleapis/java-spanner/commit/e96e17246bee9691171b46857806d03d1f8e19b4)) - add QueryOptions proto ([#84](https://www.github.com/googleapis/java-spanner/issues/84)) ([eb8fc37](https://www.github.com/googleapis/java-spanner/commit/eb8fc375bbd766f25966aa565e266ed972bbe818)) ##### Bug Fixes - never use credentials in combination with plain text ([#98](https://www.github.com/googleapis/java-spanner/issues/98)) ([7eb8d49](https://www.github.com/googleapis/java-spanner/commit/7eb8d49cd6c35d7f757cb89009ad16be601b77c3)) ##### Dependencies - update dependency com.google.cloud:google-cloud-core-bom to v1.93.1 ([#91](https://www.github.com/googleapis/java-spanner/issues/91)) ([29d8db8](https://www.github.com/googleapis/java-spanner/commit/29d8db8cfc9d12824b9264d0fb870049a58a9a03)) - update dependency io.opencensus:opencensus-api to v0.25.0 ([#95](https://www.github.com/googleapis/java-spanner/issues/95)) ([57f5fd0](https://www.github.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/dashboard#googleapis/java-spanner-jdbc).

googlebot added the cla: yes This 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: no This human has *not* signed the Contributor License Agreement. and removed cla: yes This human has signed the Contributor License Agreement. labels Feb 20, 2020

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

googlebot added cla: no This human has *not* signed the Contributor License Agreement. and removed cla: yes This 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-run Add this label to force Kokoro to re-run the tests. label Feb 21, 2020

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

mayurkale22 added cla: yes This human has signed the Contributor License Agreement. and removed cla: no This 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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix: multiple calls to end of span #75

Fix: multiple calls to end of span #75

mayurkale22 commented Feb 14, 2020 •

edited by skuruppu

mayurkale22 Feb 14, 2020

mayurkale22 commented Feb 14, 2020

rghetia Feb 14, 2020

mayurkale22 Feb 14, 2020

skuruppu left a comment

mayurkale22 commented Feb 19, 2020

olavloite commented Feb 20, 2020

googlebot commented Feb 20, 2020

mayurkale22 commented Feb 20, 2020

googlebot commented Feb 20, 2020

googlebot commented Feb 20, 2020

mayurkale22 commented Feb 21, 2020

olavloite left a comment

googlebot commented Feb 21, 2020

mayurkale22 commented Feb 21, 2020

mayurkale22 commented Feb 21, 2020

mayurkale22 commented Feb 25, 2020

Fix: multiple calls to end of span #75

Fix: multiple calls to end of span #75

Conversation

mayurkale22 commented Feb 14, 2020 • edited by skuruppu

mayurkale22 Feb 14, 2020

Choose a reason for hiding this comment

mayurkale22 commented Feb 14, 2020

rghetia Feb 14, 2020

Choose a reason for hiding this comment

mayurkale22 Feb 14, 2020

Choose a reason for hiding this comment

skuruppu left a comment

Choose a reason for hiding this comment

mayurkale22 commented Feb 19, 2020

olavloite commented Feb 20, 2020

googlebot commented Feb 20, 2020

mayurkale22 commented Feb 20, 2020

googlebot commented Feb 20, 2020

googlebot commented Feb 20, 2020

mayurkale22 commented Feb 21, 2020

olavloite left a comment

Choose a reason for hiding this comment

googlebot commented Feb 21, 2020

mayurkale22 commented Feb 21, 2020

mayurkale22 commented Feb 21, 2020

mayurkale22 commented Feb 25, 2020

mayurkale22 commented Feb 14, 2020 •

edited by skuruppu