[BEAM-12164]: Add sad path tests for Cloud Spanner's ReadChangeStream #16846

zoercai · 2022-02-14T07:04:26Z

Add sad path unit tests for the Cloud Spanner's ReadChangeStream, including testing for errors from the Spanner backend.

thiagotnunes · 2022-02-14T08:31:12Z

sdks/java/io/google-cloud-platform/build.gradle

  testImplementation library.java.powermock
  testImplementation library.java.powermock_mockito
  testImplementation library.java.joda_time
+  testImplementation "com.google.cloud:google-cloud-spanner:6.17.4:tests"


Can the version come from the BOM? (if you remove the version, does it work?)

Could you create a variable as the other libraries (e.g. library.java.spanner.tests)?

It doesn't work if I remove the version, but I've moved this to BeamModulePlugin.groovy with the versions defined by variables

.../google-cloud-platform/src/main/java/org/apache/beam/sdk/io/gcp/spanner/SpannerAccessor.java

...-platform/src/main/java/org/apache/beam/sdk/io/gcp/spanner/changestreams/dao/DaoFactory.java

...test/java/org/apache/beam/sdk/io/gcp/spanner/changestreams/SpannerChangeStreamErrorTest.java

thiagotnunes

Thanks, LGTM with a few nits.

Do we need to add documentation (javadocs) for the new options?

thiagotnunes · 2022-02-15T05:51:16Z

buildSrc/src/main/groovy/org/apache/beam/gradle/BeamModulePlugin.groovy

    def classgraph_version = "4.8.104"
    def errorprone_version = "2.10.0"
+    // Try to keep gax_version consistent with gax-grpc version in google_cloud_platform_libraries_bom
+    def gax_version = "2.8.1"


I think that this one will work without the version as the BOM defines a version for it (https://mvnrepository.com/artifact/com.google.cloud/libraries-bom/24.2.0)

I did try this without the version, but the URL becomes invalid if the version is not between the gax-grpc: and the :testlib on line 549. This is the error produced without the version:

Let me know if you know of another way!

That is ok, thanks for trying

.../google-cloud-platform/src/main/java/org/apache/beam/sdk/io/gcp/spanner/SpannerAccessor.java

...test/java/org/apache/beam/sdk/io/gcp/spanner/changestreams/SpannerChangeStreamErrorTest.java

thiagotnunes · 2022-02-15T05:54:26Z

...test/java/org/apache/beam/sdk/io/gcp/spanner/changestreams/SpannerChangeStreamErrorTest.java

+
+    Statement changeStreamQueryStatement =
+        Statement.newBuilder(
+                "SELECT * FROM READ_my-change-stream(   start_timestamp => @startTimestamp,   end_timestamp => @endTimestamp,   partition_token => @partitionToken,   read_options => null,   heartbeat_milliseconds => @heartbeatMillis)")


nit: we could remove the extra spacing here (probably spotless added it when merging lines into one)

This is actually how it looks in the actual request statement, I believe the test will fail if I take out the spaces — https://github.com/googleapis/java-spanner/blob/main/google-cloud-spanner/src/test/java/com/google/cloud/spanner/MockSpannerServiceImpl.java#L113

thiagotnunes · 2022-02-15T05:56:15Z

You should change the title to match the JIRA ticket [BEAM-12164]: <title>

codecov · 2022-02-15T06:11:32Z

Codecov Report

Merging #16846 (0e12e60) into master (8bf68ad) will increase coverage by 8.99%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master   #16846      +/-   ##
==========================================
+ Coverage   74.63%   83.63%   +8.99%     
==========================================
  Files         655      453     -202     
  Lines       82237    62471   -19766     
==========================================
- Hits        61380    52245    -9135     
+ Misses      19861    10226    -9635     
+ Partials      996        0     -996

Impacted Files	Coverage Δ
sdks/python/apache_beam/io/jdbc.py	`78.26% <0.00%> (-6.74%)`	⬇️
...s/python/apache_beam/runners/portability/stager.py	`85.54% <0.00%> (-2.82%)`	⬇️
...apache_beam/runners/portability/portable_runner.py	`75.87% <0.00%> (-0.27%)`	⬇️
setup.py	`0.00% <0.00%> (ø)`
sdks/python/apache_beam/transforms/core.py	`92.64% <0.00%> (ø)`
sdks/python/apache_beam/portability/common_urns.py	`100.00% <0.00%> (ø)`
...python/apache_beam/runners/direct/direct_runner.py	`93.92% <0.00%> (ø)`
...hon/apache_beam/runners/worker/bundle_processor.py	`93.39% <0.00%> (ø)`
...on/apache_beam/examples/dataframe/flight_delays.py	`27.90% <0.00%> (ø)`
...apache_beam/portability/api/beam_runner_api_pb2.py	`100.00% <0.00%> (ø)`
... and 220 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 8bf68ad...0e12e60. Read the comment docs.

zoercai · 2022-02-17T03:08:04Z

Hey @pabloem could you please review this PR?

zoercai · 2022-02-17T03:33:50Z

@thiagotnunes I've made a PR (#16879) with the javadocs which I'll rebase once this PR is merged.

pabloem · 2022-02-25T07:02:47Z

once again I'm sorry about the delay. Taking a look now!

pabloem · 2022-02-25T07:12:25Z

.../google-cloud-platform/src/main/java/org/apache/beam/sdk/io/gcp/spanner/SpannerAccessor.java

      builder.setEmulatorHost(emulatorHost.get());
+      if (spannerConfig.getIsLocalChannelProvider() != null
+          && spannerConfig.getIsLocalChannelProvider().get()) {
+        builder.setChannelProvider(LocalChannelProvider.create(emulatorHost.get()));


is there any chance of passing the LocalChannelProvider from the test to avoid depending on gax_grpc_test in the main package? It's not mandatory, but it would make life easier for like 1% of users, who, may have version clashes with that at some point : )

pabloem

LGTM! Merging this. Javadoc is missing as properly pointed out by others.

kennknowles · 2022-10-19T18:28:37Z

...test/java/org/apache/beam/sdk/io/gcp/spanner/changestreams/SpannerChangeStreamErrorTest.java

+              .withInclusiveEndAt(after3Seconds));
+      pipeline.run().waitUntilFinish();
+    } finally {
+      assertThat(


Today this value is always zero. It seems very sensitive to internal details. Can we change it so that the assertion is checking correct results?

kennknowles · 2022-10-19T19:20:53Z

sdks/java/io/google-cloud-platform/build.gradle

  testImplementation library.java.powermock
  testImplementation library.java.powermock_mockito
  testImplementation library.java.joda_time
+  testImplementation library.java.google_cloud_spanner_test


This is wrong. "test" dependencies indicate that they are test suites. They are not for use as "test-related libraries". On a technical level, their dependency resolution does not work that way and you just got lucky this worked.

"test" scope jars are just for when you want to actually run someone else's tests

github-actions bot added gcp io java labels Feb 14, 2022

zoercai added 6 commits February 14, 2022 18:12

Add tests and config for retry

5dbee76

lint

4b99191

add tests

835bb07

lint

feb1bd3

Delete tests not passing

0c82dda

Rebase on apache beam master

8909673

zoercai force-pushed the sad-tests branch from acd1edf to 8909673 Compare February 14, 2022 07:12

thiagotnunes reviewed Feb 14, 2022

View reviewed changes

review changes

16ce8d8

github-actions bot added the build label Feb 15, 2022

thiagotnunes reviewed Feb 15, 2022

View reviewed changes

zoercai changed the title ~~test: Add sad path tests for Cloud Spanner's ReadChangeStream~~ [BEAM-12164]: Add sad path tests for Cloud Spanner's ReadChangeStream Feb 15, 2022

review changes

361e6da

thiagotnunes approved these changes Feb 17, 2022

View reviewed changes

zoercai mentioned this pull request Feb 17, 2022

[BEAM-12164] Add javadocs to SpannerConfig #16879

Merged

fix checkstyle

0e12e60

pabloem reviewed Feb 25, 2022

View reviewed changes

pabloem merged commit 47ab260 into apache:master Feb 25, 2022

hengfengli mentioned this pull request Feb 28, 2022

[BEAM-12164]: display the metadata table name for Spanner Change Streams Connector #16845

Merged

kennknowles reviewed Oct 19, 2022

View reviewed changes

[BEAM-12164]: Add sad path tests for Cloud Spanner's ReadChangeStream #16846

[BEAM-12164]: Add sad path tests for Cloud Spanner's ReadChangeStream #16846

Uh oh!

Conversation

zoercai commented Feb 14, 2022

Uh oh!

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!

thiagotnunes left a comment

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!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thiagotnunes commented Feb 15, 2022

Uh oh!

codecov bot commented Feb 15, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

zoercai commented Feb 17, 2022

Uh oh!

zoercai commented Feb 17, 2022

Uh oh!

pabloem commented Feb 25, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pabloem left a comment

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

codecov bot commented Feb 15, 2022 •

edited

Loading