fix failing HikariCP integration test #468
Conversation
sergiyvamz
changed the title
sergiyvamz
force-pushed
the
writer-failover-properties
branch
from
9627073 to
e537560
Compare
sergiyvamz
changed the title
rework AwsWrapperDataSource
sergiyvamz
force-pushed
the
writer-failover-properties
branch
2 times, most recently
from
7f97ca2 to
6a8931a
Compare
| // Configure the driver-specific data source: | ||
| Properties targetDataSourceProps = new Properties(); | ||
| targetDataSourceProps.setProperty("serverName", "db-identifier.cluster-XYZ.us-east-2.rds.amazonaws.com"); | ||
| targetDataSourceProps.setProperty("databaseName", "employees"); |
There was a problem hiding this comment.
In ReadWriteSplittingSpringJdbcTemplateExample we set the database by calling ds.setDatabase instead of setting it on the target data source props like in this example. Do both ways accomplish the same thing? Should we use the same way in both examples to make things clearer for the user?
There was a problem hiding this comment.
Good point. Both approaches work for AwsWrapperDataSource. Probably the simplest one when database is set with ds.setDatabase() is preferable.
| } | ||
| } | ||
|
|
||
| // Identify what connection provider to use. |
There was a problem hiding this comment.
nit: should we change this comment since we have a ConnectionProvider class but that class is not what we're referring to here? eg Identify what target data source to use
There was a problem hiding this comment.
Do you mean explicitly mention DataSourceConnectionProvider and DriverConnectionProvider?
| final @Nullable String databaseName) throws SQLException { | ||
|
|
||
| if (StringUtils.isNullOrEmpty(jdbcProtocol) || StringUtils.isNullOrEmpty(serverName)) { | ||
| throw new SQLException(Messages.get("ConnectionUrlBuilder.missingJdbcProtocol")); |
There was a problem hiding this comment.
Why does the error message say we are missing the jdbc protocol if the jdbc protocol is defined but the server name is not?
There was a problem hiding this comment.
The message says either jdbc protocol or server name is missing. ConnectionUrlBuilder.missingJdbcProtocol might be not the best message ID since it doesn't mention server name.
| final Properties props) throws SQLException { | ||
|
|
||
| if (StringUtils.isNullOrEmpty(jdbcProtocol)) { | ||
| if (StringUtils.isNullOrEmpty(jdbcProtocol) || hostSpec == null) { |
There was a problem hiding this comment.
Same question, why does the error message say we are missing the jdbc protocol if it is defined but the host spec is null?
| copy.put(this.databasePropertyName, PropertyDefinition.DATABASE.getString(props)); | ||
| } | ||
|
|
||
| copy.setProperty("url", url); |
There was a problem hiding this comment.
how is this property used?
There was a problem hiding this comment.
This method is not in use and it's deprecated. We just assume that target data source has url property.
wrapper/src/test/java/integration/refactored/container/ProxyHelper.java
Outdated
Show resolved
Hide resolved
| while ((instanceIDs.size() | ||
| != testRequest.getNumOfInstances() | ||
| while ((instanceIDs.size() != testRequest.getNumOfInstances() | ||
| || instanceIDs.size() == 0 |
There was a problem hiding this comment.
| || instanceIDs.size() == 0 | |
| || instanceIDs.isEmpty() |
wrapper/src/test/java/software/amazon/jdbc/ds/AwsWrapperDataSourceTest.java
Outdated
Show resolved
Hide resolved
…lper.java Co-authored-by: Karen <[email protected]>
…urceTest.java Co-authored-by: Karen <[email protected]>
4 participants
Summary
Additional Reviewers
@karenc-bq
@aaron-congo
@aaronchung-bitquill
@crystall-bitquill
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.