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

Jump to bottom

feat: support RPC priority for JDBC connections and statements #1548

Merged
merged 6 commits into from Nov 15, 2021
Merged

feat: support RPC priority for JDBC connections and statements #1548

merged 6 commits into from Nov 15, 2021

Conversation

rahul2393
Copy link
Contributor

@rahul2393 rahul2393 commented Nov 11, 2021 •
edited by thiagotnunes

Issue
This PR will support setting RPC priority from connection URL and statements.

Fixes googleapis/java-spanner-jdbc#656

@rahul2393 rahul2393 requested review from a team as code owners November 11, 2021 07:40
@google-cla google-cla bot added the cla: no This human has *not* signed the Contributor License Agreement. label Nov 11, 2021
@product-auto-label product-auto-label bot added the api: spanner Issues related to the googleapis/java-spanner API. label Nov 11, 2021
@rahul2393 rahul2393 added cla: no This human has *not* signed the Contributor License Agreement. and removed cla: no This human has *not* signed the Contributor License Agreement. labels Nov 11, 2021
@google-cla google-cla bot 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 Nov 11, 2021
olavloite
olavloite reviewed Nov 11, 2021
View reviewed changes
google-cloud-spanner/src/main/java/com/google/cloud/spanner/connection/Connection.java Outdated Show resolved Hide resolved
google-cloud-spanner/src/main/java/com/google/cloud/spanner/connection/Connection.java Outdated Show resolved Hide resolved
google-cloud-spanner/src/main/java/com/google/cloud/spanner/connection/Connection.java Outdated Show resolved Hide resolved
google-cloud-spanner/src/main/java/com/google/cloud/spanner/connection/ConnectionImpl.java Outdated Show resolved Hide resolved
google-cloud-spanner/src/main/java/com/google/cloud/spanner/connection/ConnectionImpl.java Outdated Show resolved Hide resolved
google-cloud-spanner/src/main/java/com/google/cloud/spanner/connection/ConnectionImpl.java Outdated Show resolved Hide resolved
google-cloud-spanner/src/main/java/com/google/cloud/spanner/connection/ConnectionImpl.java Outdated Show resolved Hide resolved
google-cloud-spanner/src/main/java/com/google/cloud/spanner/connection/ConnectionOptions.java Outdated Show resolved Hide resolved
...oud-spanner/src/main/resources/com/google/cloud/spanner/connection/ClientSideStatements.json Outdated Show resolved Hide resolved
@rahul2393
Copy link
Contributor Author

Thanks for quick review @olavloite
Please review again, pushed all the changes as per suggestions

olavloite
olavloite reviewed Nov 11, 2021
View reviewed changes
Copy link
Collaborator

@olavloite olavloite left a comment

Choose a reason for hiding this comment

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

I think that in general this PR could use some more testing to verify that the priority that is set by an application is actually sent to Spanner. As far as I can see, the current implementation will only include a priority in a request if there is also a statement tag.

Take a look at the com.google.cloud.spanner.connection.TaggingTest for an example for how you could set up such a test. The difficult thing with something like priority (and also tagging) is Spanner itself does not return anything that indicates whether the priority was actually included in the request or not, so you cannot really use a normal system test for that. Instead, you can use a mock server and then inspect the requests that the mock server receives.

google-cloud-spanner/src/main/java/com/google/cloud/spanner/connection/Connection.java Outdated Show resolved Hide resolved
google-cloud-spanner/src/main/java/com/google/cloud/spanner/connection/ConnectionImpl.java Outdated Show resolved Hide resolved
google-cloud-spanner/src/main/java/com/google/cloud/spanner/connection/ConnectionImpl.java Outdated Show resolved Hide resolved
google-cloud-spanner/src/main/java/com/google/cloud/spanner/connection/ConnectionOptions.java Outdated Show resolved Hide resolved
@rahul2393
Copy link
Contributor Author

Added the test to assert RPCPriority and pushed the requested changes, please help review again @olavloite

olavloite
olavloite reviewed Nov 12, 2021
View reviewed changes
Copy link
Collaborator

@olavloite olavloite left a comment

Choose a reason for hiding this comment

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

Thanks for the changes! This looks mostly good to me. I have a couple of questions regarding being able to set the priority back to null after it has been set to something else.

The rest of the comments is mainly small questions / nitpicks on some of the tests.

google-cloud-spanner/src/main/java/com/google/cloud/spanner/connection/ConnectionImpl.java Outdated Show resolved Hide resolved
google-cloud-spanner/src/test/java/com/google/cloud/spanner/connection/ConnectionTest.java Outdated Show resolved Hide resolved
google-cloud-spanner/src/test/java/com/google/cloud/spanner/connection/ConnectionTest.java Outdated Show resolved Hide resolved
google-cloud-spanner/src/test/java/com/google/cloud/spanner/connection/ConnectionTest.java Outdated Show resolved Hide resolved
google-cloud-spanner/src/test/java/com/google/cloud/spanner/connection/ConnectionTest.java Show resolved Hide resolved
...loud-spanner/src/test/java/com/google/cloud/spanner/connection/RpcPriorityConverterTest.java Outdated Show resolved Hide resolved
...loud-spanner/src/test/java/com/google/cloud/spanner/connection/RpcPriorityConverterTest.java Outdated Show resolved Hide resolved
...oud-spanner/src/main/resources/com/google/cloud/spanner/connection/ClientSideStatements.json Outdated Show resolved Hide resolved
@rahul2393
Copy link
Contributor Author

@olavloite Added the requested changes, please help review.

olavloite
olavloite reviewed Nov 12, 2021
View reviewed changes
google-cloud-spanner/src/main/java/com/google/cloud/spanner/Options.java Outdated Show resolved Hide resolved
...anner/src/main/java/com/google/cloud/spanner/connection/ConnectionStatementExecutorImpl.java Outdated Show resolved Hide resolved
...oud-spanner/src/main/resources/com/google/cloud/spanner/connection/ClientSideStatements.json
@@ -383,7 +383,7 @@
"setStatement": {
"propertyName": "RPC_PRIORITY",
"separator": "=",
"allowedValues": "'(HIGH|MEDIUM|LOW)'",
"allowedValues": "'(HIGH|MEDIUM|LOW|NULL)'",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Adding a new value to allowedValues means that you also need to regenerate the tests so it will also include tests with the new value:

mvn -P generate-test-sql-scripts compile

@rahul2393
Copy link
Contributor Author

@olavloite Please help review again

olavloite
olavloite approved these changes Nov 14, 2021
View reviewed changes
Copy link
Collaborator

@olavloite olavloite 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 for your patience with me on this one.

@rahul2393
Copy link
Contributor Author

Thanks @olavloite for all the help and review, it was great learning for me, this will help me in future contributions

@thiagotnunes thiagotnunes merged commit b61a0d4 into googleapis:main Nov 15, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@thiagotnunes thiagotnunes thiagotnunes approved these changes

@olavloite olavloite olavloite approved these changes

Assignees
No one assigned
Labels
api: spanner Issues related to the googleapis/java-spanner API. cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Support for RPC priority option in connection URL properties
3 participants
@rahul2393 @thiagotnunes @olavloite