-
Notifications
You must be signed in to change notification settings - Fork 119
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: Java API to configure named connections #1604
Conversation
f09138c
to
009f14e
Compare
3a637bd
to
d159bfc
Compare
009f14e
to
dc22b8e
Compare
d159bfc
to
9b05e65
Compare
dc22b8e
to
73d9d3c
Compare
9b05e65
to
8d94514
Compare
8d94514
to
363d5b6
Compare
73d9d3c
to
c8926b2
Compare
116886f
to
e8b778d
Compare
c8926b2
to
7f2de8b
Compare
e4ecf72
to
1bb166a
Compare
ef9adf1
to
420e7e1
Compare
Converting back to draft. This got too big so I split it into several PRs: |
420e7e1
to
666e768
Compare
c7b599a
to
c0827e1
Compare
666e768
to
d5f6ca1
Compare
a173501
to
2b7800a
Compare
d5f6ca1
to
ff6454f
Compare
016381c
to
d52c0f4
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.
Two things and then we can merge this:
- Let's make sure to close all unnamed connectors
- The tests of named connectors aren't clear in how they're confirming we're using a named connector.
if (config.getNamedConnection() != null) { | ||
NamedConnector namedConnector = getNamedConnector(config); | ||
return namedConnector.connector.connect(namedConnector.config); | ||
} else { |
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.
nit else is unnecessary after a return
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.
Fixed.
return connector | ||
.getConnection(config.withConnectorConfig(connector.getConfig())) | ||
.getConnectionMetadata(refreshTimeoutMs); | ||
} else { |
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.
same nit here.
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.
Fixed.
/** Shutdown all connectors and remove the singleton instance. */ | ||
public void shutdown() { | ||
this.unnamedConnectors.forEach( | ||
(key, c) -> { |
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.
nit: could this be all in one line?
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.
Fixed.
(key, c) -> { | ||
c.close(); | ||
}); | ||
this.unnamedConnectors.clear(); |
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.
We need to close all the unnamed connectors in the connectors
map.
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.
Fixed.
ConnectionConfig namedConfig = ConnectionConfig.fromConnectionProperties(connProps); | ||
Socket socket = coreSocketFactory.connect(namedConfig); | ||
// Assert that the socket opens correctly. | ||
assertThat(readLine(socket)).isEqualTo(SERVER_MESSAGE); |
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.
Sorry I still don't understand -- how do we know the registered connector was used and not simply an unnamed connector?
d52c0f4
to
9d12ed4
Compare
In real usage, an application would first call
Connector.register()
to register the configuration for a named connection.Then the application would use the connector by adding the
cloudSqlNamedConnector
parameter to the JDBC URL:Or using JDBC connection properties, by adding the
cloudSqlNamedConnector
property:The application can stop individual named connectors, or all connectors:
The application can shutdown the connector, preventing any new connections
and stoping all connector threads.
Fixes #1226