-
Notifications
You must be signed in to change notification settings - Fork 155
Add support of passing routing parameters via driver config #334
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
5aaba5a
to
54adf2e
Compare
54adf2e
to
5421c71
Compare
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.
@zhenlineo review round completed. Made couple comments.
@@ -16,7 +16,7 @@ | |||
* See the License for the specific language governing permissions and | |||
* limitations under the License. | |||
*/ | |||
package org.neo4j.driver.v1.util; | |||
package org.neo4j.driver.internal.cluster; |
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.
Maybe org.neo4j.driver.internal.util
?
assertThat( e.getMessage(), | ||
containsString( "Failed to call 'dbms.cluster.routing.getServers' procedure on server" ) ); | ||
assertThat( e.getMessage(), containsString( "Failed to run " + | ||
"'Statement{text='CALL dbms.cluster.routing.getServers', parameters={}}' on server." ) ); |
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.
Imho user-facing message with Statement{
does not look pretty. Could we maybe concatenate a prettier message in GetServersProcedureClusterCompositionProvider
instead of using default Statement#toString()
?
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.
Let's improve it in another PR, I will merge this PR once it is green.
runner.run( mock ); | ||
|
||
// Then | ||
assertThat( runner.procedureCalled().toString(), equalTo( |
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.
Statement
as a good #equals()
method defined. I think it should be used for assertions instead of #toString()
.
Renamed to `getRoutingTable` and `withRoutingContext`
be303fc
to
870f892
Compare
Added
withRoutingParameters
in driver Config for passing in getServersV2 parameters