You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@flink.apache.org by GitBox <gi...@apache.org> on 2020/10/09 12:53:24 UTC
[GitHub] [flink] dforciea opened a new pull request #13570: [FLINK-19522][Table SQL / Ecosystem] Add auto-commit option for Table/SQL API
dforciea opened a new pull request #13570:
URL: https://github.com/apache/flink/pull/13570
This commit adds an option in the SQL API to set the auto-commit flag.
## What is the purpose of the change
Allows the user to set the auto-commit flag on the SQL api when reading data in. For postgres, this is required to be set to false for streaming to work properly.
## Brief change log
- Update JdbcReadOptions to have an auto-commit setting
- Update JdbcDynamicTableSource to read in the config option to set auto-commit
- Update unit tests and documentation
## Verifying this change
Amended JdbcReadOptions unit test to also test to ensure auto-commit can be set
## Does this pull request potentially affect one of the following parts:
- Dependencies (does it add or upgrade a dependency): no
- The public API, i.e., is any changed class annotated with `@Public(Evolving)`: no
- The serializers: no
- The runtime per-record code paths (performance sensitive): no
- Anything that affects deployment or recovery: JobManager (and its components), Checkpointing, Kubernetes/Yarn/Mesos, ZooKeeper: no
- The S3 file system connector: no
## Documentation
- Does this pull request introduce a new feature? yes
- If yes, how is the feature documented? docs
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink] wuchong merged pull request #13570: [FLINK-19522][Table SQL / Ecosystem] Add auto-commit option for Table/SQL API
Posted by GitBox <gi...@apache.org>.
wuchong merged pull request #13570:
URL: https://github.com/apache/flink/pull/13570
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink] wuchong commented on a change in pull request #13570: [FLINK-19522][Table SQL / Ecosystem] Add auto-commit option for Table/SQL API
Posted by GitBox <gi...@apache.org>.
wuchong commented on a change in pull request #13570:
URL: https://github.com/apache/flink/pull/13570#discussion_r503181259
##########
File path: docs/dev/table/connectors/jdbc.md
##########
@@ -183,7 +183,14 @@ Connector Options
<td style="word-wrap: break-word;">0</td>
<td>Integer</td>
<td>The number of rows that should be fetched from the database when reading per round trip. If the value specified is zero, then the hint is ignored.</td>
- </tr>
+ </tr>
+ <tr>
+ <td><h5>scan.auto-commit</h5></td>
+ <td>optional</td>
+ <td style="word-wrap: break-word;">(none)</td>
+ <td>Boolean</td>
+ <td>Sets the auto commit flag on the JDBC driver.</td>
Review comment:
I think the purpose of this option is addressing some specific database requirement. So having postgres as an example is great for users to understand.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink] flinkbot commented on pull request #13570: [FLINK-19522][Table SQL / Ecosystem] Add auto-commit option for Table/SQL API
Posted by GitBox <gi...@apache.org>.
flinkbot commented on pull request #13570:
URL: https://github.com/apache/flink/pull/13570#issuecomment-706164362
Thanks a lot for your contribution to the Apache Flink project. I'm the @flinkbot. I help the community
to review your pull request. We will use this comment to track the progress of the review.
## Automated Checks
Last check on commit a55035fb0a9a2aecec9e1469f30a6f991e9e9504 (Fri Oct 09 12:55:57 UTC 2020)
**Warnings:**
* **This pull request references an unassigned [Jira ticket](https://issues.apache.org/jira/browse/FLINK-19522).** According to the [code contribution guide](https://flink.apache.org/contributing/contribute-code.html), tickets need to be assigned before starting with the implementation work.
<sub>Mention the bot in a comment to re-run the automated checks.</sub>
## Review Progress
* ❓ 1. The [description] looks good.
* ❓ 2. There is [consensus] that the contribution should go into to Flink.
* ❓ 3. Needs [attention] from.
* ❓ 4. The change fits into the overall [architecture].
* ❓ 5. Overall code [quality] is good.
Please see the [Pull Request Review Guide](https://flink.apache.org/contributing/reviewing-prs.html) for a full explanation of the review process.<details>
The Bot is tracking the review progress through labels. Labels are applied according to the order of the review items. For consensus, approval by a Flink committer of PMC member is required <summary>Bot commands</summary>
The @flinkbot bot supports the following commands:
- `@flinkbot approve description` to approve one or more aspects (aspects: `description`, `consensus`, `architecture` and `quality`)
- `@flinkbot approve all` to approve all aspects
- `@flinkbot approve-until architecture` to approve everything until `architecture`
- `@flinkbot attention @username1 [@username2 ..]` to require somebody's attention
- `@flinkbot disapprove architecture` to remove an approval you gave earlier
</details>
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink] wuchong commented on a change in pull request #13570: [FLINK-19522][Table SQL / Ecosystem] Add auto-commit option for Table/SQL API
Posted by GitBox <gi...@apache.org>.
wuchong commented on a change in pull request #13570:
URL: https://github.com/apache/flink/pull/13570#discussion_r503155496
##########
File path: flink-connectors/flink-connector-jdbc/src/main/java/org/apache/flink/connector/jdbc/table/JdbcDynamicTableFactory.java
##########
@@ -109,6 +109,11 @@
.withDescription("gives the reader a hint as to the number of rows that should be fetched, from" +
" the database when reading per round trip. If the value specified is zero, then the hint is ignored. The" +
" default value is zero.");
+ private static final ConfigOption<Boolean> SCAN_AUTO_COMMIT = ConfigOptions
+ .key("scan.auto-commit")
+ .booleanType()
+ .noDefaultValue()
Review comment:
In the mailing list, I had made a mistake that I thought the `autoCommit` is false by default in `JdbcRowDataInputFormat.Builder` (but it is null by default), so I said it should be true by default. As for now, I think we don't need to add a patch to branch 1.11 because this is not a bug, but is a new feature.
I think it would be more reasonable to make auto commit default true everywhere. The behavior of JDBC connector should match JDBC driver default.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink] flinkbot edited a comment on pull request #13570: [FLINK-19522][Table SQL / Ecosystem] Add auto-commit option for Table/SQL API
Posted by GitBox <gi...@apache.org>.
flinkbot edited a comment on pull request #13570:
URL: https://github.com/apache/flink/pull/13570#issuecomment-706172557
<!--
Meta data
{
"version" : 1,
"metaDataEntries" : [ {
"hash" : "a55035fb0a9a2aecec9e1469f30a6f991e9e9504",
"status" : "SUCCESS",
"url" : "https://dev.azure.com/apache-flink/98463496-1af2-4620-8eab-a2ecc1a2e6fe/_build/results?buildId=7331",
"triggerID" : "a55035fb0a9a2aecec9e1469f30a6f991e9e9504",
"triggerType" : "PUSH"
} ]
}-->
## CI report:
* a55035fb0a9a2aecec9e1469f30a6f991e9e9504 Azure: [SUCCESS](https://dev.azure.com/apache-flink/98463496-1af2-4620-8eab-a2ecc1a2e6fe/_build/results?buildId=7331)
<details>
<summary>Bot commands</summary>
The @flinkbot bot supports the following commands:
- `@flinkbot run travis` re-run the last Travis build
- `@flinkbot run azure` re-run the last Azure build
</details>
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink] dforciea commented on a change in pull request #13570: [FLINK-19522][Table SQL / Ecosystem] Add auto-commit option for Table/SQL API
Posted by GitBox <gi...@apache.org>.
dforciea commented on a change in pull request #13570:
URL: https://github.com/apache/flink/pull/13570#discussion_r502798851
##########
File path: flink-connectors/flink-connector-jdbc/src/main/java/org/apache/flink/connector/jdbc/table/JdbcDynamicTableFactory.java
##########
@@ -109,6 +109,11 @@
.withDescription("gives the reader a hint as to the number of rows that should be fetched, from" +
" the database when reading per round trip. If the value specified is zero, then the hint is ignored. The" +
" default value is zero.");
+ private static final ConfigOption<Boolean> SCAN_AUTO_COMMIT = ConfigOptions
+ .key("scan.auto-commit")
+ .booleanType()
+ .noDefaultValue()
Review comment:
I had let it be optional so that it could use the JDBC default. But, per the JDBC spec the default is true, so your suggestion makes sense. I'll make that change.
In the email thread, you had made a comment about setting auto commit to false by default within `JdbcRowDataInputFormat.Builder` in the 1.11 branch so that it would do the right thing for scans in postgres. However, that would conflict with setting the default to be true here. If someone expected that behavior in a 1.11 patch and then we add this, the behavior would revert back if they don't specify this option explicitly.
In general, I think that leaving auto commit enabled is the right thing to do. However, this is a config option specifically for doing scans, so I could see an argument for letting the default be false in this case. But, I can also see a good argument for making it match the JDBC driver default. I would suggest whatever is decided that it should be consistent, so if we set the default to be true here we probably shouldn't change `JdbcRowDataInputFormat.Builder`.
Let me know which direction you'd like to go with this.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink] wuchong commented on a change in pull request #13570: [FLINK-19522][Table SQL / Ecosystem] Add auto-commit option for Table/SQL API
Posted by GitBox <gi...@apache.org>.
wuchong commented on a change in pull request #13570:
URL: https://github.com/apache/flink/pull/13570#discussion_r502756591
##########
File path: docs/dev/table/connectors/jdbc.md
##########
@@ -183,7 +183,14 @@ Connector Options
<td style="word-wrap: break-word;">0</td>
<td>Integer</td>
<td>The number of rows that should be fetched from the database when reading per round trip. If the value specified is zero, then the hint is ignored.</td>
- </tr>
+ </tr>
+ <tr>
+ <td><h5>scan.auto-commit</h5></td>
+ <td>optional</td>
+ <td style="word-wrap: break-word;">(none)</td>
+ <td>Boolean</td>
+ <td>Sets the auto commit flag on the JDBC driver.</td>
Review comment:
Maybe it would be better to have a link to explain what is the auto commit? For example: https://docs.oracle.com/javase/tutorial/jdbc/basics/transactions.html#commit_transactions?
However the above link only explains the behavior for DMLs, maybe some explanation for what it is used for scan would be better.
##########
File path: flink-connectors/flink-connector-jdbc/src/main/java/org/apache/flink/connector/jdbc/table/JdbcDynamicTableFactory.java
##########
@@ -109,6 +109,11 @@
.withDescription("gives the reader a hint as to the number of rows that should be fetched, from" +
" the database when reading per round trip. If the value specified is zero, then the hint is ignored. The" +
" default value is zero.");
+ private static final ConfigOption<Boolean> SCAN_AUTO_COMMIT = ConfigOptions
+ .key("scan.auto-commit")
+ .booleanType()
+ .noDefaultValue()
Review comment:
I think auto commit mode is not a three-valued (`true`, `false`, `null`), it should be either enabled or disabled. Besides, it should be enabled by default, because this is the default behavior of JDBC.
So what about changing the default value to `true` and also update the member virables from `Boolean` to primitive `boolean`?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink] flinkbot edited a comment on pull request #13570: [FLINK-19522][Table SQL / Ecosystem] Add auto-commit option for Table/SQL API
Posted by GitBox <gi...@apache.org>.
flinkbot edited a comment on pull request #13570:
URL: https://github.com/apache/flink/pull/13570#issuecomment-706172557
<!--
Meta data
{
"version" : 1,
"metaDataEntries" : [ {
"hash" : "a55035fb0a9a2aecec9e1469f30a6f991e9e9504",
"status" : "SUCCESS",
"url" : "https://dev.azure.com/apache-flink/98463496-1af2-4620-8eab-a2ecc1a2e6fe/_build/results?buildId=7331",
"triggerID" : "a55035fb0a9a2aecec9e1469f30a6f991e9e9504",
"triggerType" : "PUSH"
}, {
"hash" : "38017c0f4f0ec3235e8b1db79c8efa258916287c",
"status" : "PENDING",
"url" : "https://dev.azure.com/apache-flink/98463496-1af2-4620-8eab-a2ecc1a2e6fe/_build/results?buildId=7460",
"triggerID" : "38017c0f4f0ec3235e8b1db79c8efa258916287c",
"triggerType" : "PUSH"
} ]
}-->
## CI report:
* a55035fb0a9a2aecec9e1469f30a6f991e9e9504 Azure: [SUCCESS](https://dev.azure.com/apache-flink/98463496-1af2-4620-8eab-a2ecc1a2e6fe/_build/results?buildId=7331)
* 38017c0f4f0ec3235e8b1db79c8efa258916287c Azure: [PENDING](https://dev.azure.com/apache-flink/98463496-1af2-4620-8eab-a2ecc1a2e6fe/_build/results?buildId=7460)
<details>
<summary>Bot commands</summary>
The @flinkbot bot supports the following commands:
- `@flinkbot run travis` re-run the last Travis build
- `@flinkbot run azure` re-run the last Azure build
</details>
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink] dforciea commented on a change in pull request #13570: [FLINK-19522][Table SQL / Ecosystem] Add auto-commit option for Table/SQL API
Posted by GitBox <gi...@apache.org>.
dforciea commented on a change in pull request #13570:
URL: https://github.com/apache/flink/pull/13570#discussion_r502797174
##########
File path: docs/dev/table/connectors/jdbc.md
##########
@@ -183,7 +183,14 @@ Connector Options
<td style="word-wrap: break-word;">0</td>
<td>Integer</td>
<td>The number of rows that should be fetched from the database when reading per round trip. If the value specified is zero, then the hint is ignored.</td>
- </tr>
+ </tr>
+ <tr>
+ <td><h5>scan.auto-commit</h5></td>
+ <td>optional</td>
+ <td style="word-wrap: break-word;">(none)</td>
+ <td>Boolean</td>
+ <td>Sets the auto commit flag on the JDBC driver.</td>
Review comment:
Good idea. I had actually at first made a comment about the postgres JDBC driver behavior with scans, since I have run across this multiple times, but had removed it since it was vendor-specific. Any thoughts about including a comment about that?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink] flinkbot edited a comment on pull request #13570: [FLINK-19522][Table SQL / Ecosystem] Add auto-commit option for Table/SQL API
Posted by GitBox <gi...@apache.org>.
flinkbot edited a comment on pull request #13570:
URL: https://github.com/apache/flink/pull/13570#issuecomment-706172557
<!--
Meta data
{
"version" : 1,
"metaDataEntries" : [ {
"hash" : "a55035fb0a9a2aecec9e1469f30a6f991e9e9504",
"status" : "PENDING",
"url" : "https://dev.azure.com/apache-flink/98463496-1af2-4620-8eab-a2ecc1a2e6fe/_build/results?buildId=7331",
"triggerID" : "a55035fb0a9a2aecec9e1469f30a6f991e9e9504",
"triggerType" : "PUSH"
} ]
}-->
## CI report:
* a55035fb0a9a2aecec9e1469f30a6f991e9e9504 Azure: [PENDING](https://dev.azure.com/apache-flink/98463496-1af2-4620-8eab-a2ecc1a2e6fe/_build/results?buildId=7331)
<details>
<summary>Bot commands</summary>
The @flinkbot bot supports the following commands:
- `@flinkbot run travis` re-run the last Travis build
- `@flinkbot run azure` re-run the last Azure build
</details>
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink] flinkbot edited a comment on pull request #13570: [FLINK-19522][Table SQL / Ecosystem] Add auto-commit option for Table/SQL API
Posted by GitBox <gi...@apache.org>.
flinkbot edited a comment on pull request #13570:
URL: https://github.com/apache/flink/pull/13570#issuecomment-706172557
<!--
Meta data
{
"version" : 1,
"metaDataEntries" : [ {
"hash" : "a55035fb0a9a2aecec9e1469f30a6f991e9e9504",
"status" : "SUCCESS",
"url" : "https://dev.azure.com/apache-flink/98463496-1af2-4620-8eab-a2ecc1a2e6fe/_build/results?buildId=7331",
"triggerID" : "a55035fb0a9a2aecec9e1469f30a6f991e9e9504",
"triggerType" : "PUSH"
}, {
"hash" : "38017c0f4f0ec3235e8b1db79c8efa258916287c",
"status" : "UNKNOWN",
"url" : "TBD",
"triggerID" : "38017c0f4f0ec3235e8b1db79c8efa258916287c",
"triggerType" : "PUSH"
} ]
}-->
## CI report:
* a55035fb0a9a2aecec9e1469f30a6f991e9e9504 Azure: [SUCCESS](https://dev.azure.com/apache-flink/98463496-1af2-4620-8eab-a2ecc1a2e6fe/_build/results?buildId=7331)
* 38017c0f4f0ec3235e8b1db79c8efa258916287c UNKNOWN
<details>
<summary>Bot commands</summary>
The @flinkbot bot supports the following commands:
- `@flinkbot run travis` re-run the last Travis build
- `@flinkbot run azure` re-run the last Azure build
</details>
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink] flinkbot commented on pull request #13570: [FLINK-19522][Table SQL / Ecosystem] Add auto-commit option for Table/SQL API
Posted by GitBox <gi...@apache.org>.
flinkbot commented on pull request #13570:
URL: https://github.com/apache/flink/pull/13570#issuecomment-706172557
<!--
Meta data
{
"version" : 1,
"metaDataEntries" : [ {
"hash" : "a55035fb0a9a2aecec9e1469f30a6f991e9e9504",
"status" : "UNKNOWN",
"url" : "TBD",
"triggerID" : "a55035fb0a9a2aecec9e1469f30a6f991e9e9504",
"triggerType" : "PUSH"
} ]
}-->
## CI report:
* a55035fb0a9a2aecec9e1469f30a6f991e9e9504 UNKNOWN
<details>
<summary>Bot commands</summary>
The @flinkbot bot supports the following commands:
- `@flinkbot run travis` re-run the last Travis build
- `@flinkbot run azure` re-run the last Azure build
</details>
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink] flinkbot commented on pull request #13570: [FLINK-19522][Table SQL / Ecosystem] Add auto-commit option for Table/SQL API
Posted by GitBox <gi...@apache.org>.
flinkbot commented on pull request #13570:
URL: https://github.com/apache/flink/pull/13570#issuecomment-706164362
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink] wuchong commented on a change in pull request #13570: [FLINK-19522][Table SQL / Ecosystem] Add auto-commit option for Table/SQL API
Posted by GitBox <gi...@apache.org>.
wuchong commented on a change in pull request #13570:
URL: https://github.com/apache/flink/pull/13570#discussion_r503155496
##########
File path: flink-connectors/flink-connector-jdbc/src/main/java/org/apache/flink/connector/jdbc/table/JdbcDynamicTableFactory.java
##########
@@ -109,6 +109,11 @@
.withDescription("gives the reader a hint as to the number of rows that should be fetched, from" +
" the database when reading per round trip. If the value specified is zero, then the hint is ignored. The" +
" default value is zero.");
+ private static final ConfigOption<Boolean> SCAN_AUTO_COMMIT = ConfigOptions
+ .key("scan.auto-commit")
+ .booleanType()
+ .noDefaultValue()
Review comment:
In the mailing list, I had made a mistake that I thought the `autoCommit` is false by default in `JdbcRowDataInputFormat.Builder` (but it is null by default), so I said it should be true by default. As for now, I think we don't need to add a patch to branch 1.11 because this is not a bug, but is a new feature.
I think it would be more reasonable to make auto commit default true everywhere.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink] flinkbot edited a comment on pull request #13570: [FLINK-19522][Table SQL / Ecosystem] Add auto-commit option for Table/SQL API
Posted by GitBox <gi...@apache.org>.
flinkbot edited a comment on pull request #13570:
URL: https://github.com/apache/flink/pull/13570#issuecomment-706172557
<!--
Meta data
{
"version" : 1,
"metaDataEntries" : [ {
"hash" : "a55035fb0a9a2aecec9e1469f30a6f991e9e9504",
"status" : "DELETED",
"url" : "https://dev.azure.com/apache-flink/98463496-1af2-4620-8eab-a2ecc1a2e6fe/_build/results?buildId=7331",
"triggerID" : "a55035fb0a9a2aecec9e1469f30a6f991e9e9504",
"triggerType" : "PUSH"
}, {
"hash" : "38017c0f4f0ec3235e8b1db79c8efa258916287c",
"status" : "SUCCESS",
"url" : "https://dev.azure.com/apache-flink/98463496-1af2-4620-8eab-a2ecc1a2e6fe/_build/results?buildId=7460",
"triggerID" : "38017c0f4f0ec3235e8b1db79c8efa258916287c",
"triggerType" : "PUSH"
} ]
}-->
## CI report:
* 38017c0f4f0ec3235e8b1db79c8efa258916287c Azure: [SUCCESS](https://dev.azure.com/apache-flink/98463496-1af2-4620-8eab-a2ecc1a2e6fe/_build/results?buildId=7460)
<details>
<summary>Bot commands</summary>
The @flinkbot bot supports the following commands:
- `@flinkbot run travis` re-run the last Travis build
- `@flinkbot run azure` re-run the last Azure build
</details>
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org