You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@storm.apache.org by rahuljain373 <gi...@git.apache.org> on 2017/07/31 07:49:41 UTC
[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 <ra...@adobe.com>
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm pull request #2248: STORM-2028: Fix for uprooting the JDBC client exce...
Posted by revans2 <gi...@git.apache.org>.
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<Column> row = createRow(1, "frank");
+ List<List<Column>> rows = new ArrayList<List<Column>>();
+ 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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm pull request #2248: STORM-2028: Fix for uprooting the JDBC client exce...
Posted by HeartSaVioR <gi...@git.apache.org>.
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm pull request #2248: STORM-2028: Fix for uprooting the JDBC client exce...
Posted by RPCMoritz <gi...@git.apache.org>.
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<String, Object> configMap;
+
+ public MockConnectionProvider(Map<String, Object> 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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm issue #2248: STORM-2028: Fix for uprooting the JDBC client exceptions ...
Posted by RPCMoritz <gi...@git.apache.org>.
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm issue #2248: STORM-2028: Fix for uprooting the JDBC client exceptions ...
Posted by revans2 <gi...@git.apache.org>.
Github user revans2 commented on the issue:
https://github.com/apache/storm/pull/2248
To me it looks good, just one question about the test (and this is mostly for my own knowledge).
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm pull request #2248: STORM-2028: Fix for uprooting the JDBC client exce...
Posted by RPCMoritz <gi...@git.apache.org>.
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<String, Object> configMap;
+
+ public MockConnectionProvider(Map<String, Object> 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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm pull request #2248: STORM-2028: Fix for uprooting the JDBC client exce...
Posted by HeartSaVioR <gi...@git.apache.org>.
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm pull request #2248: STORM-2028: Fix for uprooting the JDBC client exce...
Posted by RPCMoritz <gi...@git.apache.org>.
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<String, Object> configMap;
+
+ public MockConnectionProvider(Map<String, Object> 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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm issue #2248: STORM-2028: Fix for uprooting the JDBC client exceptions ...
Posted by rahuljain373 <gi...@git.apache.org>.
Github user rahuljain373 commented on the issue:
https://github.com/apache/storm/pull/2248
@HeartSaVioR Squashed the commits and fixed the styling 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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm issue #2248: STORM-2028: Fix for uprooting the JDBC client exceptions ...
Posted by HeartSaVioR <gi...@git.apache.org>.
Github user HeartSaVioR commented on the issue:
https://github.com/apache/storm/pull/2248
+1
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm pull request #2248: STORM-2028: Fix for uprooting the JDBC client exce...
Posted by RPCMoritz <gi...@git.apache.org>.
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm pull request #2248: STORM-2028: Fix for uprooting the JDBC client exce...
Posted by rahuljain373 <gi...@git.apache.org>.
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm pull request #2248: STORM-2028: Fix for uprooting the JDBC client exce...
Posted by rahuljain373 <gi...@git.apache.org>.
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<Column> row = createRow(1, "frank");
+ List<List<Column>> rows = new ArrayList<List<Column>>();
+ 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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm pull request #2248: STORM-2028: Fix for uprooting the JDBC client exce...
Posted by HeartSaVioR <gi...@git.apache.org>.
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm pull request #2248: STORM-2028: Fix for uprooting the JDBC client exce...
Posted by rahuljain373 <gi...@git.apache.org>.
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<Column> row = createRow(1, "frank");
+ List<List<Column>> rows = new ArrayList<List<Column>>();
+ 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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm pull request #2248: STORM-2028: Fix for uprooting the JDBC client exce...
Posted by rahuljain373 <gi...@git.apache.org>.
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 <ra...@adobe.com>
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm pull request #2248: STORM-2028: Fix for uprooting the JDBC client exce...
Posted by RPCMoritz <gi...@git.apache.org>.
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<Column> row = createRow(1, "frank");
+ List<List<Column>> rows = new ArrayList<List<Column>>();
+ 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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm issue #2248: STORM-2028: Fix for uprooting the JDBC client exceptions ...
Posted by rahuljain373 <gi...@git.apache.org>.
Github user rahuljain373 commented on the issue:
https://github.com/apache/storm/pull/2248
@RPCMoritz Should i ask someone for merge??
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm pull request #2248: STORM-2028: Fix for uprooting the JDBC client exce...
Posted by asfgit <gi...@git.apache.org>.
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---