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: support RPC priority for JDBC connections and statements #1548

Merged
merged 6 commits into from Nov 15, 2021

Conversation

rahul2393
Copy link
Contributor

@rahul2393 rahul2393 commented Nov 11, 2021

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

Fixes googleapis/java-spanner-jdbc#656

@rahul2393 rahul2393 requested review from as code owners Nov 11, 2021
@google-cla google-cla bot added the cla: no label Nov 11, 2021
@product-auto-label product-auto-label bot added the api: spanner label Nov 11, 2021
@rahul2393 rahul2393 added cla: no and removed cla: no labels Nov 11, 2021
@google-cla google-cla bot added cla: yes and removed cla: no labels Nov 11, 2021
@rahul2393
Copy link
Contributor Author

@rahul2393 rahul2393 commented Nov 11, 2021

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

Copy link
Contributor

@olavloite olavloite left a comment

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.

@rahul2393
Copy link
Contributor Author

@rahul2393 rahul2393 commented Nov 12, 2021

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

Copy link
Contributor

@olavloite olavloite left a comment

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.

@rahul2393
Copy link
Contributor Author

@rahul2393 rahul2393 commented Nov 12, 2021

@olavloite Added the requested changes, please help review.

@@ -383,7 +383,7 @@
"setStatement": {
"propertyName": "RPC_PRIORITY",
"separator": "=",
"allowedValues": "'(HIGH|MEDIUM|LOW)'",
"allowedValues": "'(HIGH|MEDIUM|LOW|NULL)'",
Copy link
Contributor

@olavloite olavloite Nov 12, 2021

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

@rahul2393 rahul2393 commented Nov 12, 2021

@olavloite Please help review again

Copy link
Contributor

@olavloite olavloite left a comment

LGTM, thanks for your patience with me on this one.

@rahul2393
Copy link
Contributor Author

@rahul2393 rahul2393 commented Nov 14, 2021

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
19 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: spanner cla: yes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants