feat: support RPC priority for JDBC connections and statements#1548
feat: support RPC priority for JDBC connections and statements#1548thiagotnunes merged 6 commits intogoogleapis:mainfrom rahul2393:jdbc-rpc-priority
Conversation
|
Thanks for quick review @olavloite |
olavloite
left a comment
There was a problem hiding this 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.
|
Added the test to assert RPCPriority and pushed the requested changes, please help review again @olavloite |
olavloite
left a comment
There was a problem hiding this 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.
|
@olavloite Added the requested changes, please help review. |
| "propertyName": "RPC_PRIORITY", | ||
| "separator": "=", | ||
| "allowedValues": "'(HIGH|MEDIUM|LOW)'", | ||
| "allowedValues": "'(HIGH|MEDIUM|LOW|NULL)'", |
There was a problem hiding this comment.
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
|
@olavloite Please help review again |
olavloite
left a comment
There was a problem hiding this comment.
LGTM, thanks for your patience with me on this one.
|
Thanks @olavloite for all the help and review, it was great learning for me, this will help me in future contributions |
Issue
This PR will support setting RPC priority from connection URL and statements.
Fixes googleapis/java-spanner-jdbc#656