[GitHub] storm pull request #2248: STORM-2028: Fix for uprooting the JDBC client exce...
Github user asfgit closed the pull request at: https://github.com/apache/storm/pull/2248 --- 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 rahuljain373 commented on a diff in the pull request: https://github.com/apache/storm/pull/2248#discussion_r131534160 --- 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 -- It's thrown from closeConnection via throw new IllegalStateException(finalException). If an exception is embedded into another exception, it will be thrown as MultipleFailureException. --- 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 revans2 commented on a diff in the pull request: https://github.com/apache/storm/pull/2248#discussion_r131263697 --- 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 -- I am not super familiar with MultipleFailureException. I though that `thrown.expect(MultipleFailureException.class)` will verify that an instance of MultipleFailureException is thrown by `client.executeInsertQuery...` but because it is not a part of our code I must be wrong. Is it being treated like a wild card then? --- 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 HeartSaVioR commented on a diff in the pull request: https://github.com/apache/storm/pull/2248#discussion_r131020922 --- 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: " + e.getMessage()); +} --- End diff -- `else` after `}` with a space, `{` after `else` with a space. --- 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 HeartSaVioR commented on a diff in the pull request: https://github.com/apache/storm/pull/2248#discussion_r131020932 --- 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: " + e.getMessage()); +} +else +{ +finalException = new RuntimeException("Failed to close connection", e); +} } } + +if (finalException != null) --- End diff -- `{` after `)` with a space. --- 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 HeartSaVioR commented on a diff in the pull request: https://github.com/apache/storm/pull/2248#discussion_r131020840 --- 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) --- End diff -- `{` after `)` with a space. --- 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 rahuljain373 commented on a diff in the pull request: https://github.com/apache/storm/pull/2248#discussion_r130399722 --- 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 -- Yup, RuntimeException is expected with earlier 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_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. ---
[GitHub] storm pull request #2248: STORM-2028: Fix for uprooting the JDBC client exce...
GitHub user rahuljain373 reopened a pull request: https://github.com/apache/storm/pull/2248 STORM-2028: Fix for uprooting the JDBC client exceptions in case of s⦠Fix for uprooting the JDBC client exceptions in case of subsequent connection closure issues You can merge this pull request into a Git repository by running: $ git pull https://github.com/rahuljain373/storm STORM-2028 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/storm/pull/2248.patch To close this pull request, make a commit to your master/trunk branch with (at least) the following in the commit message: This closes #2248 commit 024d2694cca2c98daed769fb82f93c945e715a10 Author: Rahul Jain Date: 2017-07-31T07:44:39Z STORM-2028: Fix for uprooting the JDBC client exceptions in case of subsequent connection closure issues --- 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 rahuljain373 closed the pull request at: https://github.com/apache/storm/pull/2248 --- 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 rahuljain373 opened a pull request: https://github.com/apache/storm/pull/2248 STORM-2028: Fix for uprooting the JDBC client exceptions in case of s⦠Fix for uprooting the JDBC client exceptions in case of subsequent connection closure issues You can merge this pull request into a Git repository by running: $ git pull https://github.com/rahuljain373/storm STORM-2028 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/storm/pull/2248.patch To close this pull request, make a commit to your master/trunk branch with (at least) the following in the commit message: This closes #2248 commit 024d2694cca2c98daed769fb82f93c945e715a10 Author: Rahul Jain Date: 2017-07-31T07:44:39Z STORM-2028: Fix for uprooting the JDBC client exceptions in case of subsequent connection closure issues --- 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. ---