You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@calcite.apache.org by el...@apache.org on 2017/10/09 16:07:15 UTC
calcite-avatica git commit: [CALCITE-2003] Remove global
synchronization on openConnection
Repository: calcite-avatica
Updated Branches:
refs/heads/master d19740921 -> fcb1342a4
[CALCITE-2003] Remove global synchronization on openConnection
Try to fix some TravisCI issues that have recently cropped up.
Closes apache/calcite-avatica#17
Project: http://git-wip-us.apache.org/repos/asf/calcite-avatica/repo
Commit: http://git-wip-us.apache.org/repos/asf/calcite-avatica/commit/fcb1342a
Tree: http://git-wip-us.apache.org/repos/asf/calcite-avatica/tree/fcb1342a
Diff: http://git-wip-us.apache.org/repos/asf/calcite-avatica/diff/fcb1342a
Branch: refs/heads/master
Commit: fcb1342a404a38b14ddefb1f4d81fd2c4c953718
Parents: d197409
Author: Josh Elser <el...@apache.org>
Authored: Fri Oct 6 13:27:46 2017 -0400
Committer: Josh Elser <el...@apache.org>
Committed: Mon Oct 9 12:02:29 2017 -0400
----------------------------------------------------------------------
.travis.yml | 10 ++--
.../apache/calcite/avatica/jdbc/JdbcMeta.java | 35 +++++++++----
.../calcite/avatica/jdbc/JdbcMetaTest.java | 54 ++++++++++++++++++++
3 files changed, 87 insertions(+), 12 deletions(-)
----------------------------------------------------------------------
http://git-wip-us.apache.org/repos/asf/calcite-avatica/blob/fcb1342a/.travis.yml
----------------------------------------------------------------------
diff --git a/.travis.yml b/.travis.yml
index d54d884..034f285 100644
--- a/.travis.yml
+++ b/.travis.yml
@@ -17,9 +17,11 @@
# limitations under the License.
#
language: java
-jdk:
- - oraclejdk8
- - oraclejdk7
+matrix:
+ fast_finish: true
+ include:
+ - jdk: oraclejdk8
+ - jdk: openjdk7
branches:
only:
- master
@@ -32,6 +34,7 @@ install:
- mvn -V install -DskipTests -Dmaven.javadoc.skip=true
script:
# Print surefire output to the console instead of files
+ - unset _JAVA_OPTIONS
- mvn -Dsurefire.useFile=false verify
git:
depth: 10000
@@ -39,4 +42,5 @@ sudo: false
cache:
directories:
- $HOME/.m2
+sudo: required
# End .travis.yml
http://git-wip-us.apache.org/repos/asf/calcite-avatica/blob/fcb1342a/server/src/main/java/org/apache/calcite/avatica/jdbc/JdbcMeta.java
----------------------------------------------------------------------
diff --git a/server/src/main/java/org/apache/calcite/avatica/jdbc/JdbcMeta.java b/server/src/main/java/org/apache/calcite/avatica/jdbc/JdbcMeta.java
index 348d48f..38f92ff 100644
--- a/server/src/main/java/org/apache/calcite/avatica/jdbc/JdbcMeta.java
+++ b/server/src/main/java/org/apache/calcite/avatica/jdbc/JdbcMeta.java
@@ -67,6 +67,7 @@ import java.util.List;
import java.util.Map;
import java.util.Objects;
import java.util.Properties;
+import java.util.concurrent.ConcurrentMap;
import java.util.concurrent.TimeUnit;
import java.util.concurrent.atomic.AtomicInteger;
@@ -216,6 +217,11 @@ public class JdbcMeta implements ProtobufMeta {
}
// For testing purposes
+ protected Cache<String, Connection> getConnectionCache() {
+ return connectionCache;
+ }
+
+ // For testing purposes
protected Cache<Integer, StatementInfo> getStatementCache() {
return statementCache;
}
@@ -610,19 +616,30 @@ public class JdbcMeta implements ProtobufMeta {
fullInfo.putAll(info);
}
- synchronized (this) {
- try {
- if (connectionCache.asMap().containsKey(ch.id)) {
- throw new RuntimeException("Connection already exists: " + ch.id);
- }
- Connection conn = DriverManager.getConnection(url, fullInfo);
- connectionCache.put(ch.id, conn);
- } catch (SQLException e) {
- throw new RuntimeException(e);
+ final ConcurrentMap<String, Connection> cacheAsMap = connectionCache.asMap();
+ if (cacheAsMap.containsKey(ch.id)) {
+ throw new RuntimeException("Connection already exists: " + ch.id);
+ }
+ // Avoid global synchronization of connection opening
+ try {
+ Connection conn = createConnection(url, fullInfo);
+ Connection loadedConn = cacheAsMap.putIfAbsent(ch.id, conn);
+ // Race condition: someone beat us to storing the connection in the cache.
+ if (loadedConn != null) {
+ conn.close();
+ throw new RuntimeException("Connection already exists: " + ch.id);
}
+ } catch (SQLException e) {
+ throw new RuntimeException(e);
}
}
+ // Visible for testing
+ protected Connection createConnection(String url, Properties info) throws SQLException {
+ // Allows simpler testing of openConnection
+ return DriverManager.getConnection(url, info);
+ }
+
@Override public void closeConnection(ConnectionHandle ch) {
Connection conn = connectionCache.getIfPresent(ch.id);
if (conn == null) {
http://git-wip-us.apache.org/repos/asf/calcite-avatica/blob/fcb1342a/server/src/test/java/org/apache/calcite/avatica/jdbc/JdbcMetaTest.java
----------------------------------------------------------------------
diff --git a/server/src/test/java/org/apache/calcite/avatica/jdbc/JdbcMetaTest.java b/server/src/test/java/org/apache/calcite/avatica/jdbc/JdbcMetaTest.java
index d84fd29..f3ebc8a 100644
--- a/server/src/test/java/org/apache/calcite/avatica/jdbc/JdbcMetaTest.java
+++ b/server/src/test/java/org/apache/calcite/avatica/jdbc/JdbcMetaTest.java
@@ -31,10 +31,14 @@ import java.sql.ParameterMetaData;
import java.sql.PreparedStatement;
import java.sql.ResultSetMetaData;
import java.sql.SQLException;
+import java.util.Collections;
+import java.util.Map;
+import java.util.Properties;
import java.util.UUID;
import java.util.concurrent.atomic.AtomicInteger;
import static org.hamcrest.CoreMatchers.is;
+import static org.junit.Assert.assertEquals;
import static org.junit.Assert.assertThat;
import static org.junit.Assert.fail;
@@ -118,6 +122,56 @@ public class JdbcMetaTest {
// Verify we called setMaxRows with the right value
Mockito.verify(statement).setMaxRows(maxRows);
}
+
+ @Test public void testConcurrentConnectionOpening() throws Exception {
+ final Map<String, String> properties = Collections.emptyMap();
+ final Connection conn = Mockito.mock(Connection.class);
+ // Override JdbcMeta to shim in a fake Connection object. Irrelevant for the test
+ JdbcMeta meta = new JdbcMeta("jdbc:url") {
+ @Override protected Connection createConnection(String url, Properties info) {
+ return conn;
+ }
+ };
+
+ ConnectionHandle ch1 = new ConnectionHandle("id1");
+ meta.openConnection(ch1, properties);
+
+ assertEquals(conn, meta.getConnectionCache().getIfPresent(ch1.id));
+ try {
+ meta.openConnection(ch1, properties);
+ fail("Should not be allowed to open two connections with the same ID");
+ } catch (RuntimeException e) {
+ // pass
+ }
+ // Cached object should not change
+ assertEquals(conn, meta.getConnectionCache().getIfPresent(ch1.id));
+ }
+
+ @Test public void testRacingConnectionOpens() throws Exception {
+ final Map<String, String> properties = Collections.emptyMap();
+ final Connection conn1 = Mockito.mock(Connection.class);
+ final Connection conn2 = Mockito.mock(Connection.class);
+ final ConnectionHandle ch1 = new ConnectionHandle("id1");
+ // Override JdbcMeta to shim in a fake Connection object. Irrelevant for the test
+ JdbcMeta meta = new JdbcMeta("jdbc:url") {
+ @Override protected Connection createConnection(String url, Properties info) {
+ // Hack to mimic the race condition of a connection being cached by another thread.
+ getConnectionCache().put(ch1.id, conn1);
+ // Return our "newly created" connectino
+ return conn2;
+ }
+ };
+
+ try {
+ meta.openConnection(ch1, properties);
+ fail("Should see an exception when the cache already contained our connection after"
+ + " creating it");
+ } catch (RuntimeException e) {
+ // pass
+ }
+ // Our opened connection should get closed when this race condition happens
+ Mockito.verify(conn2).close();
+ }
}
// End JdbcMetaTest.java