You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@flink.apache.org by sbcd90 <gi...@git.apache.org> on 2016/01/26 21:21:16 UTC
[GitHub] flink pull request: [FLINK-3292]Fix for Bug in flink-jdbc. Not all...
GitHub user sbcd90 opened a pull request:
https://github.com/apache/flink/pull/1551
[FLINK-3292]Fix for Bug in flink-jdbc. Not all JDBC drivers supported
Hello,
Here is the fix for issue FLINK-3292. Kindly review & merge.
Thanks & regards,
Subhobrata
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/sbcd90/flink master
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/flink/pull/1551.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 #1551
----
commit 8f1ab49c823d53d4b46eb57789bdca29533bb37e
Author: Subhobrata Dey <sb...@gmail.com>
Date: 2016-01-26T20:18:49Z
[FLINK-3292]Fix for Bug in flink-jdbc. Not all JDBC drivers supported
----
---
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] flink pull request: [FLINK-3292]Fix for Bug in flink-jdbc. Not all...
Posted by zentol <gi...@git.apache.org>.
Github user zentol commented on the pull request:
https://github.com/apache/flink/pull/1551#issuecomment-176157025
looks good. +1 to 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] flink pull request: [FLINK-3292]Fix for Bug in flink-jdbc. Not all...
Posted by zentol <gi...@git.apache.org>.
Github user zentol commented on the pull request:
https://github.com/apache/flink/pull/1551#issuecomment-175619319
In that case i would suggest the following:
- change the default values to TYPE_FORWARD_ONLY and CONCUR_READ_ONLY to be in line with createStatement()
-allow the user to set either one of these
---
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] flink pull request: [FLINK-3292]Fix for Bug in flink-jdbc. Not all...
Posted by zentol <gi...@git.apache.org>.
Github user zentol commented on a diff in the pull request:
https://github.com/apache/flink/pull/1551#discussion_r50955346
--- Diff: flink-batch-connectors/flink-jdbc/src/main/java/org/apache/flink/api/java/io/jdbc/JDBCInputFormat.java ---
@@ -92,6 +94,15 @@ public void open(InputSplit ignored) throws IOException {
}
}
+ private Statement createStatement() throws SQLException {
+ if(resultSetType == 0 || resultSetConcurrency == 0) {
--- End diff --
this line might lead to confusion when a user only sets either the type or concurrency.
IMO we should check with && here, require the user to set both type and concurrency at the same time, and throw a meaningful exception if only one these is 0.
---
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] flink pull request: [FLINK-3292]Fix for Bug in flink-jdbc. Not all...
Posted by sbcd90 <gi...@git.apache.org>.
Github user sbcd90 commented on the pull request:
https://github.com/apache/flink/pull/1551#issuecomment-176153352
Hello zentol,
I dont see any updates from your side. Can you kindly let me know if this PR is good to be merged so that I can close the issue?
---
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] flink pull request: [FLINK-3292]Fix for Bug in flink-jdbc. Not all...
Posted by zentol <gi...@git.apache.org>.
Github user zentol commented on a diff in the pull request:
https://github.com/apache/flink/pull/1551#discussion_r50993131
--- Diff: flink-batch-connectors/flink-jdbc/src/test/java/org/apache/flink/api/java/io/jdbc/JDBCOutputFormatTest.java ---
@@ -19,10 +19,7 @@
package org.apache.flink.api.java.io.jdbc;
import java.io.IOException;
-import java.sql.Connection;
-import java.sql.DriverManager;
-import java.sql.SQLException;
-import java.sql.Statement;
+import java.sql.*;
--- End diff --
we don't allow star imports.
---
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] flink pull request: [FLINK-3292]Fix for Bug in flink-jdbc. Not all...
Posted by sbcd90 <gi...@git.apache.org>.
Github user sbcd90 commented on the pull request:
https://github.com/apache/flink/pull/1551#issuecomment-175653356
Hello zentol,
Can you check the code changes now? The changes look exactly similar to what you suggest. Can you let me know if this can be merged? or do I provide a new PR?
---
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] flink pull request: [FLINK-3292]Fix for Bug in flink-jdbc. Not all...
Posted by sbcd90 <gi...@git.apache.org>.
Github user sbcd90 commented on the pull request:
https://github.com/apache/flink/pull/1551#issuecomment-175589833
Hello zentol,
I made changes according to your post. Kindly review now.
---
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] flink pull request: [FLINK-3292]Fix for Bug in flink-jdbc. Not all...
Posted by zentol <gi...@git.apache.org>.
Github user zentol commented on the pull request:
https://github.com/apache/flink/pull/1551#issuecomment-175666661
if you can revert the changes to the imports it's good to merge IMO.
you don't have to create a new PR. you can squash the commits if you wish, otherwise I'll do it when merging.
---
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] flink pull request: [FLINK-3292]Fix for Bug in flink-jdbc. Not all...
Posted by StephanEwen <gi...@git.apache.org>.
Github user StephanEwen commented on the pull request:
https://github.com/apache/flink/pull/1551#issuecomment-176188709
Thank for the contribution and review!
Will merge this...
---
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] flink pull request: [FLINK-3292]Fix for Bug in flink-jdbc. Not all...
Posted by sbcd90 <gi...@git.apache.org>.
Github user sbcd90 commented on the pull request:
https://github.com/apache/flink/pull/1551#issuecomment-175592466
Hello zentol,
To answer your initial question,
```
is there any way to check which createStatement method is supported? what happens if you use the wrong statement?
```
I went into the Jdbc driver code & found that both methods are supported, however, with the ones defaulted in the code,
ResultSet.TYPE_SCROLL_INSENSITIVE
ResultSet.CONCUR_READ_ONLY
I got this error,
```
java.sql.SQLException: Result set type is TYPE_FORWARD_ONLY
```
I think both variants should be supported & it is up to the api user to specify which variant they should go for. Hence, this PR. Kindly let me know if the changes in code are fine now.
---
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] flink pull request: [FLINK-3292]Fix for Bug in flink-jdbc. Not all...
Posted by zentol <gi...@git.apache.org>.
Github user zentol commented on the pull request:
https://github.com/apache/flink/pull/1551#issuecomment-175486665
is there any way to check which createStatement method is supported? what happens if you use the wrong statement?
---
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] flink pull request: [FLINK-3292]Fix for Bug in flink-jdbc. Not all...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/flink/pull/1551
---
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] flink pull request: [FLINK-3292]Fix for Bug in flink-jdbc. Not all...
Posted by sbcd90 <gi...@git.apache.org>.
Github user sbcd90 commented on the pull request:
https://github.com/apache/flink/pull/1551#issuecomment-175810825
Hello zentol,
I have squashed all changes. Can you please merge now?
---
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.
---