-
Notifications
You must be signed in to change notification settings - Fork 131
feat: support RPC priority for JDBC connections and statements #1548
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
Conversation
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
Thanks for quick review @olavloite |
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.
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
Added the test to assert RPCPriority and pushed the requested changes, please help review again @olavloite |
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.
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
@olavloite Added the requested changes, please help review. |
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
@@ -383,7 +383,7 @@ | |||
"setStatement": { | |||
"propertyName": "RPC_PRIORITY", | |||
"separator": "=", | |||
"allowedValues": "'(HIGH|MEDIUM|LOW)'", | |||
"allowedValues": "'(HIGH|MEDIUM|LOW|NULL)'", |
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.
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 |
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, 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