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

feat: add QueryOptions samples for JDBC #2347

Merged

Conversation

olavloite
Copy link
Contributor

@olavloite olavloite commented Mar 6, 2020

Adds samples for QueryOptions for Cloud Spanner JDBC. This PR also adds basic infrastructure for samples for Cloud Spanner JDBC.

The PR currently fails to build as the changes that are needed have not yet been added to the JDBC driver.

@olavloite olavloite requested review from skuruppu, a team and lesv March 6, 2020 13:43
@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Mar 6, 2020
Copy link
Contributor

@lesv lesv left a comment

Choose a reason for hiding this comment

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

There is only one or two things that need any action on.

Overall I like this. Let me know what we might need to do the get testing to pass. It will probably be easier if we use EnvVar's instead of java properties. And we really don't want customers to consider using -SNAPSHOT's.

spanner/jdbc/README.md Show resolved Hide resolved
spanner/jdbc/pom.xml Outdated Show resolved Hide resolved
<dependency>
<groupId>com.google.cloud</groupId>
<artifactId>google-cloud-spanner-jdbc</artifactId>
<version>1.13.1-SNAPSHOT</version>
Copy link
Contributor

Choose a reason for hiding this comment

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

Don't use -SNAPSHOT's

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll change that as soon as a version of the JDBC driver with support for QueryOptions has been released. Until then this PR won't build anyways, so it can't be merged by accident either.

spanner/jdbc/pom.xml Outdated Show resolved Hide resolved
@olavloite
Copy link
Contributor Author

There is only one or two things that need any action on.

Overall I like this. Let me know what we might need to do the get testing to pass. It will probably be easier if we use EnvVar's instead of java properties. And we really don't want customers to consider using -SNAPSHOT's.

@lesv
This PR will only build once googleapis/java-spanner-jdbc#76 has been merged. There's nothing actionable for you or your team to make this pass at this moment.

@skuruppu skuruppu added the api: spanner Issues related to the Spanner API. label Mar 20, 2020
@skuruppu
Copy link
Contributor

@olavloite if you could look at the tests that would be great. Looks like there are some failures.

@olavloite
Copy link
Contributor Author

@lesv

Overall I like this. Let me know what we might need to do the get testing to pass. It will probably be easier if we use EnvVar's instead of java properties. And we really don't want customers to consider using -SNAPSHOT's.

All the required dependencies have now been merged, so it should now be possible to get the test cases up and running. For that the integration test needs access to a Spanner instance and database. I assumed that the Java system properties spanner.test.instance and spanner.test.database would automatically have been set by the build environment, as those are also used by the SpannerSampleIT, but apparently it's not. I have no specific preference for either a Java system property or an Environment Variable. Could you arrange for one or the other being set for the integration tests of this build? If it's an EnvVar, I'll modify the IT accordingly to use that.

spanner/jdbc/pom.xml Outdated Show resolved Hide resolved
spanner/jdbc/pom.xml Outdated Show resolved Hide resolved
@lesv
Copy link
Contributor

lesv commented Mar 25, 2020

Great - other than it doesn't compile.

@lesv lesv added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Mar 25, 2020
@kokoro-team kokoro-team removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Mar 25, 2020
@olavloite
Copy link
Contributor Author

Great - other than it doesn't compile.

It depends on System.getProperty("spanner.test.instance") and System.getProperty("spanner.sample.database") which are available for the normal Spanner samples build, but apparently not for this build. I'm open for any other way way of getting a reference to a valid test instance, such as the previously mentioned EnvVar.

@lesv
Copy link
Contributor

lesv commented Mar 25, 2020

You can hard code these in your test. If you need me to do something in java-docs-samples-testing, let me know. We have default-instance in us-central1 and the following DB's:


  | Name | CPU utilization | Size |  
-- | -- | -- | -- | --
  | mysample-4ca24702-bd50-41a4-a | 0.62% | 474 B |  
  | mysample-73af71b0-b212-4794-b | 0.38% | 603 B |  
  | quickstart-db | 0.32% | 89 B |  
  | restored-mysample-16ae8df9-7ec | 0.23% | 1.81 KB |  
  | sejzn-3d2384a5-1344-4bff-8 | 0.29% | 13.46 KB |  
  | vzdqp-da80840c-38c5-430b-a

@olavloite olavloite merged commit 46e8278 into GoogleCloudPlatform:master Mar 25, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: spanner Issues related to the Spanner API. cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants