Skip to content

Commit

Permalink
Minor bugfixes.
Browse files Browse the repository at this point in the history
  • Loading branch information
@brettwooldridge
brettwooldridge committed Jul 6, 2015
1 parent 7dfcb26 commit b18c786
Show file tree
Hide file tree
Showing 4 changed files with 18 additions and 16 deletions.
6 changes: 3 additions & 3 deletions src/main/java/com/zaxxer/hikari/HikariConfig.java
View file
Expand Up @@ -79,7 +79,7 @@ public class HikariConfig implements HikariConfigMXBean
private String transactionIsolationName;
private String username;
private boolean isAutoCommit;
private boolean isReadOnly;
private Boolean isReadOnly;

This comment has been minimized.

Sign in to view

Copy link
@dotta

dotta Aug 18, 2015

@brettwooldridge Hi Brett, after bumping the hikaricp version to 2.4.0 in Play a couple of tests started to fail because the type of isReadOnly has changed (see PR playframework/playframework#5003). This is easy to fix, but I'm wondering why the change.

This comment has been minimized.

Sign in to view

Copy link
@brettwooldridge

brettwooldridge Aug 18, 2015

Author Owner

This was changed to work around a VoltDB issue #340 . However, if you read the last comment, made just last week, I believe the change should be backed out.

I assume that the failure you are seeing is the result of calling isReadOnly() without having called setReadOnly()? I have to admit it is not a use case we envisioned and was not covered by a unit test.

This comment has been minimized.

Sign in to view

Copy link
@brettwooldridge

brettwooldridge Aug 19, 2015

Author Owner

@dotta Change reverted and committed to 2.4.1. VoltDB users will have to get relief for this issue through a correction in the VoltDB JDBC driver.

This comment has been minimized.

Sign in to view

Copy link
@dotta

dotta Aug 19, 2015

Sounds good. One question though, are you planning to include this change in the 2.4.x stream? I'm asking because this is a binary incompatible change.

This comment has been minimized.

Sign in to view

Copy link
@brettwooldridge

brettwooldridge Aug 19, 2015

Author Owner

That was my intention (to include it in the 2.4.x stream). From my perspective the change in 2.4.0 resulted in an unintentional break in binary compatibility with 2.3.x, and therefore was a bug.

From the Semantic Versioning FAQ:

Q. What do I do if I accidentally release a backwards incompatible change as a
minor version?

A. As soon as you realize that you've broken the Semantic Versioning spec, fix
the problem and release a new minor version that corrects the problem and
restores backwards compatibility. Even under this circumstance, it is
unacceptable to modify versioned releases. If it's appropriate, document the
offending version and inform your users of the problem so that they are
aware of the offending version.

This comment has been minimized.

Sign in to view

Copy link
@dotta

dotta Aug 19, 2015

That's good enough for me. Then I'll be eagerly waiting for 2.4.1 :-) Thanks Brett!
private boolean isInitializationFailFast;
private boolean isIsolateInternalQueries;
private boolean isRegisterMbeans;
Expand Down Expand Up @@ -529,12 +529,12 @@ public void addHealthCheckProperty(String key, String value)
healthCheckProperties.setProperty(key, value);
}

public boolean isReadOnly()
public Boolean isReadOnly()
{
return isReadOnly;
}

public void setReadOnly(boolean readOnly)
public void setReadOnly(Boolean readOnly)
{
this.isReadOnly = readOnly;
}
Expand Down
2 changes: 1 addition & 1 deletion src/main/java/com/zaxxer/hikari/pool/HikariPool.java
View file
Expand Up @@ -176,7 +176,7 @@ public final Connection getConnection(final long hardTimeout) throws SQLExceptio
}

final long now = clockSource.currentTime();
if (bagEntry.evicted || (clockSource.elapsedMillis(bagEntry.lastAccess, now) > ALIVE_BYPASS_WINDOW_MS && !poolElf.isConnectionAlive(bagEntry.connection))) {
if (bagEntry.evicted || (clockSource.elapsedMillis(bagEntry.lastAccess, now) > ALIVE_BYPASS_WINDOW_MS && !poolElf.isConnectionAlive(bagEntry.connection, lastConnectionFailure))) {
closeConnection(bagEntry, "(connection evicted or dead)"); // Throw away the dead connection and try again
timeout = hardTimeout - clockSource.elapsedMillis(startTime, now);
}
Expand Down
24 changes: 13 additions & 11 deletions src/main/java/com/zaxxer/hikari/pool/PoolElf.java
View file
Expand Up @@ -14,6 +14,7 @@
import java.util.concurrent.ThreadFactory;
import java.util.concurrent.ThreadPoolExecutor;
import java.util.concurrent.TimeUnit;
import java.util.concurrent.atomic.AtomicReference;

import javax.management.MBeanServer;
import javax.management.ObjectName;
Expand Down Expand Up @@ -44,7 +45,7 @@ public final class PoolElf
private final HikariConfig config;
private final String poolName;
private final String catalog;
private final boolean isReadOnly;
private final Boolean isReadOnly;
private final boolean isAutoCommit;
private final boolean isUseJdbc4Validation;
private final boolean isIsolateInternalQueries;
Expand Down Expand Up @@ -154,12 +155,7 @@ else if (jdbcUrl != null && dataSource == null) {
* Setup a connection initial state.
*
* @param connection a Connection
* @param connectionTimeout
* @param initSql
* @param isAutoCommit auto-commit state
* @param isReadOnly read-only state
* @param transactionIsolation transaction isolation
* @param catalog default catalog
* @param connectionTimeout the connection timeout
* @throws SQLException thrown from driver
*/
void setupConnection(final Connection connection, final long connectionTimeout) throws SQLException
Expand All @@ -172,7 +168,9 @@ void setupConnection(final Connection connection, final long connectionTimeout)
transactionIsolation = (transactionIsolation < 0 ? connection.getTransactionIsolation() : transactionIsolation);

connection.setAutoCommit(isAutoCommit);
connection.setReadOnly(isReadOnly);
if (isReadOnly != null) {
connection.setReadOnly(isReadOnly);
}

if (transactionIsolation != connection.getTransactionIsolation()) {
connection.setTransactionIsolation(transactionIsolation);
Expand All @@ -191,9 +189,10 @@ void setupConnection(final Connection connection, final long connectionTimeout)
* Check whether the connection is alive or not.
*
* @param connection the connection to test
* @param lastConnectionFailure last connection failure
* @return true if the connection is alive, false if it is not alive or we timed out
*/
boolean isConnectionAlive(final Connection connection)
boolean isConnectionAlive(final Connection connection, AtomicReference<Throwable> lastConnectionFailure)
{
try {
int timeoutSec = (int) TimeUnit.MILLISECONDS.toSeconds(validationTimeout);
Expand All @@ -220,14 +219,15 @@ boolean isConnectionAlive(final Connection connection)
return true;
}
catch (SQLException e) {
lastConnectionFailure.set(e);
LOGGER.warn("Exception during alive check, Connection ({}) declared dead.", connection, e);
return false;
}
}

void resetConnectionState(final PoolBagEntry poolEntry) throws SQLException
{
if (poolEntry.isReadOnly != isReadOnly) {
if (isReadOnly != null && poolEntry.isReadOnly != isReadOnly) {
poolEntry.connection.setReadOnly(isReadOnly);
}

Expand All @@ -251,8 +251,10 @@ void resetConnectionState(final PoolBagEntry poolEntry) throws SQLException

void resetPoolEntry(final PoolBagEntry poolEntry)
{
if (isReadOnly != null) {
poolEntry.setReadOnly(isReadOnly);
}
poolEntry.setCatalog(catalog);
poolEntry.setReadOnly(isReadOnly);
poolEntry.setAutoCommit(isAutoCommit);
poolEntry.setNetworkTimeout(networkTimeout);
poolEntry.setTransactionIsolation(transactionIsolation);
Expand Down
2 changes: 1 addition & 1 deletion src/main/java/com/zaxxer/hikari/util/PropertyElf.java
View file
Expand Up @@ -151,7 +151,7 @@ private static void setProperty(Object target, String propName, Object propValue
else if (paramClass == long.class) {
writeMethod.invoke(target, Long.parseLong(propValue.toString()));
}
else if (paramClass == boolean.class) {
else if (paramClass == boolean.class || paramClass == Boolean.class) {
writeMethod.invoke(target, Boolean.parseBoolean(propValue.toString()));
}
else if (paramClass == String.class) {
Expand Down

0 comments on commit b18c786

Please sign in to comment.