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 support for QueryOptions #76
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM with some minor comments.
One request, if you could update the description of this PR to contain the information about the env variable and the precedence, that would be great. Example PR
src/test/java/com/google/cloud/spanner/jdbc/ConnectionImplTest.java
Outdated
Show resolved
Hide resolved
src/test/java/com/google/cloud/spanner/jdbc/ConnectionImplTest.java
Outdated
Show resolved
Hide resolved
src/test/java/com/google/cloud/spanner/jdbc/ConnectionImplTest.java
Outdated
Show resolved
Hide resolved
src/test/java/com/google/cloud/spanner/jdbc/ConnectionTest.java
Outdated
Show resolved
Hide resolved
src/test/java/com/google/cloud/spanner/jdbc/ConnectionTest.java
Outdated
Show resolved
Hide resolved
src/test/java/com/google/cloud/spanner/jdbc/it/ITJdbcQueryOptionsTest.java
Outdated
Show resolved
Hide resolved
Done. |
5cf4f49
to
4e2edfb
Compare
I merged in the v1.51.0 release which should hopefully be available on Maven by the end of your day. |
Codecov Report
@@ Coverage Diff @@
## master #76 +/- ##
============================================
+ Coverage 77.79% 77.84% +0.05%
- Complexity 1723 1732 +9
============================================
Files 60 60
Lines 5836 5881 +45
Branches 769 779 +10
============================================
+ Hits 4540 4578 +38
- Misses 923 925 +2
- Partials 373 378 +5
Continue to review full report at Codecov.
|
Looks like I can't merge in #75 until #73 can be merged in but that can only be done once googleapis/java-core#179 is in a release. Yikes, so many dependencies! |
I don't understand this failure in the Linkage Monitor when all other tests pass:
|
Your PR has attempted to merge for 3 hours. Please check that all required checks have passed, you have an automerge label, and that all your reviewers have approved the PR |
3 similar comments
Your PR has attempted to merge for 3 hours. Please check that all required checks have passed, you have an automerge label, and that all your reviewers have approved the PR |
Your PR has attempted to merge for 3 hours. Please check that all required checks have passed, you have an automerge label, and that all your reviewers have approved the PR |
Your PR has attempted to merge for 3 hours. Please check that all required checks have passed, you have an automerge label, and that all your reviewers have approved the PR |
@skuruppu I keep receiving notification emails due to the label |
Your PR was not mergeable because either one of your required status checks failed, or one of your required reviews was not approved. See required reviews for your repo here: https://github.com/googleapis/sloth/blob/master/required-checks.json |
I don't understand this Linkage Monitor error either. I just tried to execute it locally and it executed without any problems. I also don't understand why the log seems to indicate that Cloud Spanner 1.49.2 is being used, while it clearly uses 1.51.0. The output locally was:
|
@skuruppu The IT failure has been fixed, but the error in the Linkage Monitor step is preventing this from being merged. |
@chingor13 would you be able to help us with the two unexplained test failures.
Any help is appreciated. |
(Linkage Monitor developer here). The failure looks good to me; no need to fix it. GCP Libraries BOM 4.2.0 has com.google.cloud:google-cloud-spanner:1.49.2
GCP Library BOM does not look at google-cloud-spanner-jdbc's pom.xml. |
Thanks for the explanation @suztomo. When will the BOM contents be updated? This PR needs to be merged in before the end of the week as it's a blocker for a feature launch. We can't merge it currently because the PR requires the Linkage Monitor step to pass. |
I don't think of a better way than skipping the check. (repository owners can unmark "required")
@elharo Do you think of a better way than either skipping the check or waiting for google-cloud-bom release? |
The reason this check is here is that having versions out of sync like this causes hard-to-debug problems for customers. Looking at the output of the linkage monitor, this is exactly the sort of thing they're going to hit. I think @suztomo's analysis is correct. google-cloud-bom should push a new version that includes google-cloud-spanner:1.51.0. You don't need to wait for this to happen. Just ask @chingor13 to release it. Once that's out--it takes about 4 hours from start to finish, mostly waiting on Sonatype to publish--we can push a new version of libraries-bom that pulls that in. |
Thanks @elharo and @suztomo for the explanations. @chingor13 would you be able to help us with the google-cloud-bom release? |
Yep, I can do one |
Looks like java-cloud-bom/releases/tag/v0.123.2 has java-cloud-bom/pull/406. Are there any other steps required @elharo? |
we still need a libraries-bom release that incorporates this. One is in progress. |
@chingor13 Can you rerun the Linkage Monitor check? GCP Libraries BOM 4.3.0 has been released today. |
@chingor13 I see it succeeded. Thank you. |
Thanks so much @chingor13, @suztomo and @elharo for your help with this. We really appreciate it. |
🤖 I have created a release \*beep\* \*boop\* --- ## [1.15.0](https://www.github.com/googleapis/java-spanner-jdbc/compare/v1.14.0...v1.15.0) (2020-03-24) ### Features * add support for QueryOptions ([#76](https://www.github.com/googleapis/java-spanner-jdbc/issues/76)) ([b3f4cf7](https://www.github.com/googleapis/java-spanner-jdbc/commit/b3f4cf7852a2fd5f22660cc3f25a6253b9a118ab)) ### Dependencies * update spanner.version to v1.52.0 ([#95](https://www.github.com/googleapis/java-spanner-jdbc/issues/95)) ([cdf9d30](https://www.github.com/googleapis/java-spanner-jdbc/commit/cdf9d30e8ca387d87a6ffe00fa09818d135547f4)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please).
b3f4cf7 commit b3f4cf7 Author: Knut Olav Løite <koloite@gmail.com> Date: Mon Mar 23 20:04:19 2020 +0100 feat: add support for QueryOptions (#76) * feat: add support for QueryOptions * fix: review comments * deps: temporarily update because of build and test errors * deps: update to released versions * fix: use grpc 1.27.2 * fix: fix invalid query hint in IT Co-authored-by: skuruppu <skuruppu@google.com> pom.xml src/main/java/com/google/cloud/spanner/jdbc/ClientSideStatementValueConverters.java src/main/java/com/google/cloud/spanner/jdbc/Connection.java src/main/java/com/google/cloud/spanner/jdbc/ConnectionImpl.java src/main/java/com/google/cloud/spanner/jdbc/ConnectionOptions.java src/main/java/com/google/cloud/spanner/jdbc/ConnectionStatementExecutor.java src/main/java/com/google/cloud/spanner/jdbc/ConnectionStatementExecutorImpl.java src/main/java/com/google/cloud/spanner/jdbc/JdbcDriver.java src/main/java/com/google/cloud/spanner/jdbc/StatementParser.java src/main/java/com/google/cloud/spanner/jdbc/StatementResult.java src/main/resources/com/google/cloud/spanner/jdbc/ClientSideStatements.json src/test/java/com/google/cloud/spanner/jdbc/AbortedTest.java src/test/java/com/google/cloud/spanner/jdbc/AbstractConnectionImplTest.java src/test/java/com/google/cloud/spanner/jdbc/AbstractMockServerTest.java src/test/java/com/google/cloud/spanner/jdbc/ConnectionImplTest.java src/test/java/com/google/cloud/spanner/jdbc/ConnectionStatementExecutorTest.java src/test/java/com/google/cloud/spanner/jdbc/ConnectionStatementWithNoParametersTest.java src/test/java/com/google/cloud/spanner/jdbc/ConnectionStatementWithOneParameterTest.java src/test/java/com/google/cloud/spanner/jdbc/ConnectionTest.java src/test/java/com/google/cloud/spanner/jdbc/JdbcQueryOptionsTest.java src/test/java/com/google/cloud/spanner/jdbc/JdbcTypeConverterTest.java src/test/java/com/google/cloud/spanner/jdbc/it/ITJdbcQueryOptionsTest.java src/test/java/com/google/cloud/spanner/jdbc/it/ITJdbcSqlScriptTest.java src/test/resources/com/google/cloud/spanner/jdbc/ClientSideStatementsTest.sql src/test/resources/com/google/cloud/spanner/jdbc/ConnectionImplGeneratedSqlScriptTest.sql src/test/resources/com/google/cloud/spanner/jdbc/ITSqlScriptTest_TestQueryOptions.sql
10ee396 commit 10ee396 Author: release-please[bot] <55107282+release-please[bot]@users.noreply.github.com> Date: Tue Mar 24 03:26:38 2020 +0000 chore: release 1.15.0 (#96) 🤖 I have created a release \*beep\* \*boop\* --- ## [1.15.0](https://www.github.com/googleapis/java-spanner-jdbc/compare/v1.14.0...v1.15.0) (2020-03-24) ### Features * add support for QueryOptions ([#76](https://www.github.com/googleapis/java-spanner-jdbc/issues/76)) ([b3f4cf7](https://www.github.com/googleapis/java-spanner-jdbc/commit/b3f4cf7852a2fd5f22660cc3f25a6253b9a118ab)) ### Dependencies * update spanner.version to v1.52.0 ([#95](https://www.github.com/googleapis/java-spanner-jdbc/issues/95)) ([cdf9d30](https://www.github.com/googleapis/java-spanner-jdbc/commit/cdf9d30e8ca387d87a6ffe00fa09818d135547f4)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). CHANGELOG.md README.md pom.xml versions.txt
Adds the ability to set
QueryOptions
when running Cloud Spanner queries through JDBC. For now, only setting thequery_optimizer_version
is added.QueryOptions can be configured through the following mechanisms:
optimizerVersion
. The version specified here will be applied to all queries executed by the connection.SET OPTIMIZER_VERSION='<version>'
on a JDBC connection. All queries that are executed after this statement will use the version specified.SPANNER_OPTIMIZER_VERSION
environment variable.If the options are configured through multiple mechanisms then:
SET OPTIMIZER_VERSION='<version>'
will override options set at a JDBC connection URL level or environment variable level.If no options are set, the optimizer version will default to:
Depends on changes in the client library that have not yet been merged, meaning that the build of this PR will fail until the required changes have been merged.