Skip to content
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.

Sign up for GitHub

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

Create new connection provider during IP failovers #117

Merged
merged 1 commit into from
Nov 7, 2024
Merged

Create new connection provider during IP failovers #117

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 24 additions & 2 deletions worker/src/main/scala/com/lucidchart/piezo/ConnectionProvider.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,8 @@ class ConnectionProvider(props: Properties) {
@volatile
private var cachedIpWithExpiration: Option[CachedIpWithExpiration] = None

private val pool: Pool = new Pool(getIP)
@volatile
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is unnecessary since we only access it from within a synchronized block.

Copy link
Contributor Author

@aegbert5 aegbert5 Nov 7, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We read from this value outside the synchronized block on lines 113, 115, and 139, for example to get a connection:

pool.connectionProvider.get.getConnection

Thus volatile will make sure the pool is assigned completely before we attempt to get connections from it

private var pool: Pool = new Pool(getIP)

// Intended to be used only for tests. This mocks an IP failover every time a connection is retreived
private val causeFailoverEveryConnection = props.getProperty("causeFailoverEveryConnection") == "true"
Expand Down Expand Up @@ -111,7 +112,28 @@ class ConnectionProvider(props: Properties) {
val newIP: String = getIP
if (hasIpAddressChanged(pool, newIP)) {
// A failover has occurred, so we evict connections softly. New connectons look up the new IP address
pool.connectionProvider.map(_.getDataSource().getHikariPoolMXBean().softEvictConnections())
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have a theory that softEvictConnections() does not permit future connections be opened within the same pool. Creating a new HikariPool from scratch will guarantee to use the new IP address returned from the CNAME URL

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Possibly we are running into brettwooldridge/HikariCP#1836 ?

I have a theory that softEvictConnections() does not permit future connections be opened within the same pool.

I don't thilnk that is right. Nothing in the documentation indicates that behavior.

However, brettwooldridge/HikariCP#228 does recommend replacing the entire pool, so I guess that might be a better approach.

logger.info(s"IP Address has changed for ${jdbcURL}: ${pool.ip} -> ${newIP}. Attempt replacing pool...")
val optionalOldPool = synchronized {
val oldPool = pool
// check if another thread updated the pool
if (hasIpAddressChanged(pool, newIP)) {
logger.info(s"Replacing pool for ${jdbcURL}...")
pool = new Pool(newIP)
Some(oldPool)
} else {
// already up to date
logger.info(s"Pool already replaced for ${jdbcURL}")
None
}
}

// Clean up old pool so we don't leak connections to the old server
optionalOldPool.foreach { oldPool =>
oldPool.connectionProvider.foreach { provider =>
logger.info(s"Closing DB connection pool for ${jdbcURL} for failover (${oldPool.ip} -> ${pool.ip})")
provider.shutdown()
}
}
}
}
pool.connectionProvider.get.getConnection
Expand Down