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.

Sign up for GitHub

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

Jump to bottom

[BEAM-14121] Fix SpannerIO service call metrics and improve tests. #17335

Merged
merged 1 commit into from
Jun 7, 2022
Merged

[BEAM-14121] Fix SpannerIO service call metrics and improve tests. #17335

merged 1 commit into from
Jun 7, 2022

Conversation

nielm
Copy link
Contributor

@nielm nielm commented Apr 11, 2022 •
edited
Loading

Fixes issues with the ServiceCallMetrics in SpannerIO:
Fixes issue: #21635
(old-jira:BEAM-14121)

  • Fixes metric resource identifiers:
    • //spanner.googleapis.com/projects/{projectId}/instances/{instanceId}/databases/{databaseId}/tables/{tableId}
    • //spanner.googleapis.com/projects/{projectId}/instances/{instanceId}/queries/{queryName}
  • Ensure a non-null value for {queryName}
  • Fix metrics generation in SpannerIO.Read
  • Fix metrics generation in SpannerIO.Write
  • Add metrics generation to NaiveSpannerRead.
  • Refactor and improve SpannerIOReadTest to add additional coverage and verify metrics values
  • Add unit tests for NaiveSpannerRead (non-partitioned read)
  • Add metrics value verification in SpannerIOWriteTest.

GitHub Actions Tests Status (on master branch)

Build python source distribution and wheels
Python tests
Java tests

See CI.md for more information about GitHub Actions CI.

@asf-ci
Copy link

asf-ci commented Apr 11, 2022

Can one of the admins verify this patch?

4 similar comments
@asf-ci
Copy link

asf-ci commented Apr 11, 2022

Can one of the admins verify this patch?

@asf-ci
Copy link

asf-ci commented Apr 11, 2022

Can one of the admins verify this patch?

@asf-ci
Copy link

asf-ci commented Apr 11, 2022

Can one of the admins verify this patch?

@asf-ci
Copy link

asf-ci commented Apr 11, 2022

Can one of the admins verify this patch?

@nielm nielm force-pushed the request_metrics branch 2 times, most recently from 09e01cc to 6ee19b2 Compare April 11, 2022 15:46
@chamikaramj chamikaramj self-requested a review April 11, 2022 15:52
@aaltay
Copy link
Member

aaltay commented Apr 21, 2022

R: @chamikaramj

@nielm nielm force-pushed the request_metrics branch 2 times, most recently from fd76e60 to 817e43e Compare May 4, 2022 08:50
@aaltay
Copy link
Member

aaltay commented May 12, 2022

What is the next step on this PR?

@nielm
Copy link
Contributor Author

nielm commented May 13, 2022

Apologies - next steps were to rebase to master, resolving conflicts, and fix the failing checkstyle test!
Done.

@nielm nielm force-pushed the request_metrics branch 2 times, most recently from 102f1b9 to f19af58 Compare May 17, 2022 12:49
@nielm
Copy link
Contributor Author

nielm commented May 17, 2022 •
edited
Loading

Ready to merge: PreCommit test failures are unrelated to this PR: org.apache.beam.sdk.io.elasticsearch.ElasticsearchIOTest.classMethod

Caused by: java.lang.IllegalStateException: Could not find a valid Docker environment. Please see logs and check configuration

@chamikaramj
Copy link
Contributor

Ack. Thanks Niel.

chamikaramj
chamikaramj reviewed May 19, 2022
View reviewed changes
Copy link
Contributor

@chamikaramj chamikaramj left a comment

Choose a reason for hiding this comment

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

Thanks. Just nits.

...google-cloud-platform/src/main/java/org/apache/beam/sdk/io/gcp/spanner/BatchSpannerRead.java Show resolved Hide resolved
...google-cloud-platform/src/main/java/org/apache/beam/sdk/io/gcp/spanner/BatchSpannerRead.java Show resolved Hide resolved
...google-cloud-platform/src/main/java/org/apache/beam/sdk/io/gcp/spanner/BatchSpannerRead.java Show resolved Hide resolved
...ava/io/google-cloud-platform/src/main/java/org/apache/beam/sdk/io/gcp/spanner/SpannerIO.java Outdated Show resolved Hide resolved
...ava/io/google-cloud-platform/src/main/java/org/apache/beam/sdk/io/gcp/spanner/SpannerIO.java Outdated Show resolved Hide resolved
@aaltay
Copy link
Member

aaltay commented May 27, 2022

@nielm - Could you please respond to the open comments?

@nielm
Fix SpannerIO service call metrics and improve tests.
ef68037 
Adds metrics to NaiveSpannerRead.
Fixes metric resource identifiers.
Refactor and improve SpannerIOReadTest unit tests to add additional coverage.
Add unit tests for NaiveSpannerRead (non-partitioned read)
Fix metrics for SpannerIO.Write
@nielm
Copy link
Contributor Author

nielm commented May 30, 2022

@nielm - Could you please respond to the open comments?
Done - apologies I was on vacation.

@nielm
Copy link
Contributor Author

nielm commented May 30, 2022

Run Java PreCommit

@nielm
Copy link
Contributor Author

nielm commented Jun 6, 2022

Note: Java PreCommit issues are unrelated to this PR.
This can now be merged

@nielm nielm requested a review from chamikaramj June 6, 2022 14:54
chamikaramj
chamikaramj approved these changes Jun 7, 2022
View reviewed changes
Copy link
Contributor

@chamikaramj chamikaramj left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks.

@chamikaramj
Copy link
Contributor

Run Java PreCommit

@nielm
Copy link
Contributor Author

nielm commented Jun 7, 2022

re pre-commit failure in org.apache.beam.sdk.io.gcp.spanner.SpannerIOWriteExceptionHandlingTest.testExceptionHandlingForWriteGrouped

I saw this occasionally in my local testing when my local machine was under load and have been looking into it. It does seem to be a timing related flake, unrelated to this PR. Will debug and fix separately; raised #21729

> is a org.mockito.exceptions.verification.TooManyActualInvocations
Stacktrace was: org.mockito.exceptions.verification.TooManyActualInvocations: 
sleeper.sleep(<any long>);
Wanted 9 times:
-> at org.apache.beam.sdk.io.gcp.spanner.SpannerIOWriteExceptionHandlingTest.testExceptionHandlingForWriteGrouped(SpannerIOWriteExceptionHandlingTest.java:222)
But was 10 times:

@chamikaramj chamikaramj merged commit 380110e into apache:master Jun 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@chamikaramj chamikaramj chamikaramj approved these changes

Assignees
No one assigned
Labels
core gcp io java runners
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@nielm @asf-ci @aaltay @chamikaramj