[GitHub] storm issue #2248: STORM-2028: Fix for uprooting the JDBC client exceptions ...
Github user RPCMoritz commented on the issue: https://github.com/apache/storm/pull/2248 @HeartSaVioR since you helped out with this issue previously - can we get some committer attention to this PR? LGTM quickly. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] storm pull request #2248: STORM-2028: Fix for uprooting the JDBC client exce...
Github user RPCMoritz commented on a diff in the pull request:
https://github.com/apache/storm/pull/2248#discussion_r130360205
--- Diff:
external/storm-jdbc/src/test/java/org/apache/storm/jdbc/common/JdbcClientTest.java
---
@@ -92,3 +115,27 @@ public void cleanup() {
client.executeSql("drop table " + tableName);
}
}
+
+class MockConnectionProvider implements ConnectionProvider {
+
+private Map configMap;
+
+public MockConnectionProvider(Map mockCPConfigMap) {
--- End diff --
The class name could be chosen to be more descriptive --
ThrowingConnectionProvider?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm pull request #2248: STORM-2028: Fix for uprooting the JDBC client exce...
Github user RPCMoritz commented on a diff in the pull request:
https://github.com/apache/storm/pull/2248#discussion_r130362674
--- Diff:
external/storm-jdbc/src/test/java/org/apache/storm/jdbc/common/JdbcClientTest.java
---
@@ -80,6 +85,24 @@ public void testInsertAndSelect() {
Assert.assertEquals(rows, selectedRows);
}
+@Rule
+public ExpectedException thrown = ExpectedException.none();
+
+@Test
+public void testInsertConnectionError() {
+
+ConnectionProvider connectionProvider = new
MockConnectionProvider(null);
+this.client = new JdbcClient(connectionProvider, 60);
+
+List row = createRow(1, "frank");
+List> rows = new ArrayList>();
+rows.add(row);
+String query = "insert into user_details values(?,?,?)";
+
+thrown.expect(MultipleFailureException.class);
--- End diff --
Is this specific enough to fail with the previous code?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm pull request #2248: STORM-2028: Fix for uprooting the JDBC client exce...
Github user RPCMoritz commented on a diff in the pull request:
https://github.com/apache/storm/pull/2248#discussion_r130360350
--- Diff:
external/storm-jdbc/src/test/java/org/apache/storm/jdbc/common/JdbcClientTest.java
---
@@ -92,3 +115,27 @@ public void cleanup() {
client.executeSql("drop table " + tableName);
}
}
+
+class MockConnectionProvider implements ConnectionProvider {
+
+private Map configMap;
+
+public MockConnectionProvider(Map mockCPConfigMap) {
+this.configMap = mockCPConfigMap;
+}
+
+@Override
+public synchronized void prepare() {
+// To be Implemented
+}
+
+@Override
+public Connection getConnection() {
+throw new RuntimeException("connection error");
+}
+
+@Override
+public void cleanup() {
+// To be Implemented
--- End diff --
this comment isn't necessary/misguiding
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm pull request #2248: STORM-2028: Fix for uprooting the JDBC client exce...
Github user RPCMoritz commented on a diff in the pull request:
https://github.com/apache/storm/pull/2248#discussion_r130360392
--- Diff:
external/storm-jdbc/src/test/java/org/apache/storm/jdbc/common/JdbcClientTest.java
---
@@ -92,3 +115,27 @@ public void cleanup() {
client.executeSql("drop table " + tableName);
}
}
+
+class MockConnectionProvider implements ConnectionProvider {
+
+private Map configMap;
+
+public MockConnectionProvider(Map mockCPConfigMap) {
+this.configMap = mockCPConfigMap;
+}
+
+@Override
+public synchronized void prepare() {
+// To be Implemented
--- End diff --
this comment isn't necessary/misguiding
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm pull request #2248: STORM-2028: Fix for uprooting the JDBC client exce...
Github user RPCMoritz commented on a diff in the pull request:
https://github.com/apache/storm/pull/2248#discussion_r130361066
--- Diff:
external/storm-jdbc/src/main/java/org/apache/storm/jdbc/common/JdbcClient.java
---
@@ -223,13 +237,25 @@ private void
setPreparedStatementParams(PreparedStatement preparedStatement, Lis
}
}
-private void closeConnection(Connection connection) {
+private void closeConnection(Connection connection, Exception
finalException) {
if (connection != null) {
try {
connection.close();
} catch (SQLException e) {
-throw new RuntimeException("Failed to close connection",
e);
+if (finalException != null)
+{
+LOG.error("Failed to close connection");
--- End diff --
We should still log ```e``` here
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
