[GitHub] storm pull request #2248: STORM-2028: Fix for uprooting the JDBC client exce...

[asfgit]

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...

[rahuljain373]

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...

[revans2]

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...

[HeartSaVioR]

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...

[HeartSaVioR]

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...

[HeartSaVioR]

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...

[rahuljain373]

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...

[RPCMoritz]

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...

[RPCMoritz]

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...

[RPCMoritz]

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...

[RPCMoritz]

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...

[RPCMoritz]

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...

[rahuljain373]

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...

[rahuljain373]

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...

[rahuljain373]

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.
---