You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@beam.apache.org by "Polber (via GitHub)" <gi...@apache.org> on 2024/01/16 20:25:19 UTC
[PR] [YAML] Add ability to specify JDBC type to read/write [beam]
Polber opened a new pull request, #30024:
URL: https://github.com/apache/beam/pull/30024
PR adds ability to specify a JDBC type/flavor to the `ReadFromJdbc` and `WriteToJdbc` Beam YAML transforms.
------------------------
Thank you for your contribution! Follow this checklist to help us incorporate your contribution quickly and easily:
- [ ] Mention the appropriate issue in your description (for example: `addresses #123`), if applicable. This will automatically add a link to the pull request in the issue. If you would like the issue to automatically close on merging the pull request, comment `fixes #<ISSUE NUMBER>` instead.
- [ ] Update `CHANGES.md` with noteworthy changes.
- [ ] If this contribution is large, please file an Apache [Individual Contributor License Agreement](https://www.apache.org/licenses/icla.pdf).
See the [Contributor Guide](https://beam.apache.org/contribute) for more tips on [how to make review process smoother](https://github.com/apache/beam/blob/master/CONTRIBUTING.md#make-the-reviewers-job-easier).
To check the build health, please visit [https://github.com/apache/beam/blob/master/.test-infra/BUILD_STATUS.md](https://github.com/apache/beam/blob/master/.test-infra/BUILD_STATUS.md)
GitHub Actions Tests Status (on master branch)
------------------------------------------------------------------------------------------------
[](https://github.com/apache/beam/actions?query=workflow%3A%22Build+python+source+distribution+and+wheels%22+branch%3Amaster+event%3Aschedule)
[](https://github.com/apache/beam/actions?query=workflow%3A%22Python+Tests%22+branch%3Amaster+event%3Aschedule)
[](https://github.com/apache/beam/actions?query=workflow%3A%22Java+Tests%22+branch%3Amaster+event%3Aschedule)
[](https://github.com/apache/beam/actions?query=workflow%3A%22Go+tests%22+branch%3Amaster+event%3Aschedule)
See [CI.md](https://github.com/apache/beam/blob/master/CI.md) for more information about GitHub Actions CI or the [workflows README](https://github.com/apache/beam/blob/master/.github/workflows/README.md) to see a list of phrases to trigger workflows.
--
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.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [YAML] Add ability to specify JDBC type to read/write [beam]
Posted by "robertwb (via GitHub)" <gi...@apache.org>.
robertwb commented on code in PR #30024:
URL: https://github.com/apache/beam/pull/30024#discussion_r1473621804
##########
sdks/java/io/jdbc/src/main/java/org/apache/beam/sdk/io/jdbc/JdbcUtil.java:
##########
@@ -75,6 +77,23 @@ class JdbcUtil {
private static final Logger LOG = LoggerFactory.getLogger(JdbcUtil.class);
+ static final Map<String, String> JDBC_DRIVER_MAP =
+ new HashMap<>(
+ ImmutableMap.of(
+ "mysql",
+ "com.mysql.cj.jdbc.Driver",
+ "postgres",
+ "org.postgresql.Driver",
+ "oracle",
+ "oracle.jdbc.driver.OracleDriver",
+ "mssql",
+ "com.microsoft.sqlserver.jdbc.SQLServerDriver"));
+
+ @VisibleForTesting
Review Comment:
Oh, derby is an actual "fake/test" jdbc endpoint. I had totally missed this. Yes, this is fine.
--
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.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [YAML] Add ability to specify JDBC type to read/write [beam]
Posted by "Abacn (via GitHub)" <gi...@apache.org>.
Abacn commented on PR #30024:
URL: https://github.com/apache/beam/pull/30024#issuecomment-1920157165
> @Abacn Is the test failing a known issue? It is failing at container startup for mysql, but I'm not sure why my PR would affect that.
It's new issue. It's failing since Jan 19th: https://github.com/apache/beam/actions/runs/7580901305. The likely cause is new version of mysql:latest breaking the test:
```
INFO: Creating container for image: mysql:latest
Jan 19, 2024 7:48:19 AM org.testcontainers.containers.GenericContainer tryStart
INFO: Container mysql:latest is starting: 424952070b3851f1212a5c750f3d8c06ec591c9b6b4d605c4b2d2458520fa570
Jan 19, 2024 7:48:20 AM org.testcontainers.containers.JdbcDatabaseContainer waitUntilContainerStarted
INFO: Waiting for database connection to become available at jdbc:mysql://localhost:32770/test using query 'SELECT 1'
Jan 19, 2024 7:50:20 AM org.testcontainers.containers.GenericContainer tryStart
SEVERE: Could not start container
...
2024-01-19 07:48:20+00:00 [Note] [Entrypoint]: Entrypoint script for MySQL Server 8.3.0-1.el8 started.
```
--
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.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [YAML] Add ability to specify JDBC type to read/write [beam]
Posted by "Polber (via GitHub)" <gi...@apache.org>.
Polber commented on PR #30024:
URL: https://github.com/apache/beam/pull/30024#issuecomment-1920140105
@Abacn Is the test failing a known issue? It is failing at container startup for mysql, but I'm not sure why my PR would affect 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.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [YAML] Add ability to specify JDBC type to read/write [beam]
Posted by "Abacn (via GitHub)" <gi...@apache.org>.
Abacn commented on PR #30024:
URL: https://github.com/apache/beam/pull/30024#issuecomment-1934696965
Test passed:
```
PASSED [ 70%]
apache_beam/io/external/xlang_jdbcio_it_test.py::CrossLanguageJdbcIOTest::test_xlang_jdbc_write_read_1_mysql
```
--
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.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [YAML] Add ability to specify JDBC type to read/write [beam]
Posted by "Polber (via GitHub)" <gi...@apache.org>.
Polber commented on code in PR #30024:
URL: https://github.com/apache/beam/pull/30024#discussion_r1473552585
##########
sdks/java/io/jdbc/src/main/java/org/apache/beam/sdk/io/jdbc/JdbcUtil.java:
##########
@@ -75,6 +77,23 @@ class JdbcUtil {
private static final Logger LOG = LoggerFactory.getLogger(JdbcUtil.class);
+ static final Map<String, String> JDBC_DRIVER_MAP =
+ new HashMap<>(
+ ImmutableMap.of(
+ "mysql",
+ "com.mysql.cj.jdbc.Driver",
+ "postgres",
+ "org.postgresql.Driver",
+ "oracle",
+ "oracle.jdbc.driver.OracleDriver",
+ "mssql",
+ "com.microsoft.sqlserver.jdbc.SQLServerDriver"));
+
+ @VisibleForTesting
Review Comment:
A few of the tests read/write to a table (basically an E2E test) and use the derby embedded driver which I was not planning on exposing externally, but wanted a way to add it to the map for tests
--
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.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [YAML] Add ability to specify JDBC type to read/write [beam]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #30024:
URL: https://github.com/apache/beam/pull/30024#issuecomment-1933989768
Reminder, please take a look at this pr: @liferoad @damondouglas @damondouglas
--
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.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [YAML] Add ability to specify JDBC type to read/write [beam]
Posted by "damondouglas (via GitHub)" <gi...@apache.org>.
damondouglas commented on code in PR #30024:
URL: https://github.com/apache/beam/pull/30024#discussion_r1456157850
##########
sdks/java/io/jdbc/src/main/java/org/apache/beam/sdk/io/jdbc/JdbcReadSchemaTransformProvider.java:
##########
@@ -68,8 +73,29 @@ public JdbcReadSchemaTransform(JdbcReadSchemaTransformConfiguration config) {
}
protected JdbcIO.DataSourceConfiguration dataSourceConfiguration() {
+ String driverClassName = config.getDriverClassName();
+
+ if (Strings.isNullOrEmpty(driverClassName)) {
+ switch (Objects.requireNonNull(config.getJdbcType()).toLowerCase()) {
+ case "mysql":
+ driverClassName = "com.mysql.cj.jdbc.Driver";
+ break;
+ case "postgres":
+ driverClassName = "org.postgresql.Driver";
+ break;
+ case "oracle":
+ driverClassName = "oracle.jdbc.driver.OracleDriver";
+ break;
+ case "mssql":
+ driverClassName = "com.microsoft.sqlserver.jdbc.SQLServerDriver";
+ break;
Review Comment:
What if we combined this association of the driver name i.e. "mysql", "postgres", etc with the JDBCTypes above as a Map<String, String> so that future changes need not coordinate JDBCTypes above with the switch statements here. Then perhaps in this method one could do `jdbcTypesMap.contains(config.getJdbcType().toLowerCase())`.
##########
sdks/java/io/jdbc/src/main/java/org/apache/beam/sdk/io/jdbc/JdbcReadSchemaTransformProvider.java:
##########
@@ -164,12 +194,17 @@ public abstract static class JdbcReadSchemaTransformConfiguration implements Ser
public abstract String getDriverJars();
public void validate() throws IllegalArgumentException {
- if (Strings.isNullOrEmpty(getDriverClassName())) {
- throw new IllegalArgumentException("JDBC Driver class name cannot be blank.");
+ if (Strings.isNullOrEmpty(getDriverClassName()) && Strings.isNullOrEmpty(getJdbcType())) {
Review Comment:
If `getDriverClassName()` is required, why is it declared `@Nullable`?
--
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.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [YAML] Add ability to specify JDBC type to read/write [beam]
Posted by "Polber (via GitHub)" <gi...@apache.org>.
Polber commented on code in PR #30024:
URL: https://github.com/apache/beam/pull/30024#discussion_r1458126592
##########
sdks/java/io/jdbc/src/main/java/org/apache/beam/sdk/io/jdbc/JdbcReadSchemaTransformProvider.java:
##########
@@ -68,8 +73,29 @@ public JdbcReadSchemaTransform(JdbcReadSchemaTransformConfiguration config) {
}
protected JdbcIO.DataSourceConfiguration dataSourceConfiguration() {
+ String driverClassName = config.getDriverClassName();
+
+ if (Strings.isNullOrEmpty(driverClassName)) {
+ switch (Objects.requireNonNull(config.getJdbcType()).toLowerCase()) {
+ case "mysql":
+ driverClassName = "com.mysql.cj.jdbc.Driver";
+ break;
+ case "postgres":
+ driverClassName = "org.postgresql.Driver";
+ break;
+ case "oracle":
+ driverClassName = "oracle.jdbc.driver.OracleDriver";
+ break;
+ case "mssql":
+ driverClassName = "com.microsoft.sqlserver.jdbc.SQLServerDriver";
+ break;
Review Comment:
Good idea! I'll add 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.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [YAML] Add ability to specify JDBC type to read/write [beam]
Posted by "robertwb (via GitHub)" <gi...@apache.org>.
robertwb commented on code in PR #30024:
URL: https://github.com/apache/beam/pull/30024#discussion_r1473413551
##########
sdks/java/io/jdbc/src/main/java/org/apache/beam/sdk/io/jdbc/JdbcUtil.java:
##########
@@ -75,6 +77,23 @@ class JdbcUtil {
private static final Logger LOG = LoggerFactory.getLogger(JdbcUtil.class);
+ static final Map<String, String> JDBC_DRIVER_MAP =
+ new HashMap<>(
+ ImmutableMap.of(
+ "mysql",
+ "com.mysql.cj.jdbc.Driver",
+ "postgres",
+ "org.postgresql.Driver",
+ "oracle",
+ "oracle.jdbc.driver.OracleDriver",
+ "mssql",
+ "com.microsoft.sqlserver.jdbc.SQLServerDriver"));
+
+ @VisibleForTesting
Review Comment:
Are we expecting users to call this outside of tests? If not, let's just test the built-in ones. If so, make it public with 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.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [YAML] Add ability to specify JDBC type to read/write [beam]
Posted by "Polber (via GitHub)" <gi...@apache.org>.
Polber commented on code in PR #30024:
URL: https://github.com/apache/beam/pull/30024#discussion_r1458134155
##########
sdks/java/io/jdbc/src/main/java/org/apache/beam/sdk/io/jdbc/JdbcReadSchemaTransformProvider.java:
##########
@@ -164,12 +194,17 @@ public abstract static class JdbcReadSchemaTransformConfiguration implements Ser
public abstract String getDriverJars();
public void validate() throws IllegalArgumentException {
- if (Strings.isNullOrEmpty(getDriverClassName())) {
- throw new IllegalArgumentException("JDBC Driver class name cannot be blank.");
+ if (Strings.isNullOrEmpty(getDriverClassName()) && Strings.isNullOrEmpty(getJdbcType())) {
Review Comment:
It is not required. One of `driverClassName` or `jdbcType` must be provided. If `driverClassName` is not given, then the switch statement you commented on above will handle defaulting the driver.
--
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.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [YAML] Add ability to specify JDBC type to read/write [beam]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #30024:
URL: https://github.com/apache/beam/pull/30024#issuecomment-1911978089
Reminder, please take a look at this pr: @liferoad @damondouglas @damondouglas
--
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.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [YAML] Add ability to specify JDBC type to read/write [beam]
Posted by "Abacn (via GitHub)" <gi...@apache.org>.
Abacn commented on PR #30024:
URL: https://github.com/apache/beam/pull/30024#issuecomment-1934164160
What is the status of this now, @damondouglas were the change request resolved?
btw good to test Python PostCommit. Python Jdbc integration test often found break after related changes and not captured by precommit
--
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.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [YAML] Add ability to specify JDBC type to read/write [beam]
Posted by "robertwb (via GitHub)" <gi...@apache.org>.
robertwb commented on code in PR #30024:
URL: https://github.com/apache/beam/pull/30024#discussion_r1470238911
##########
sdks/java/io/jdbc/src/main/java/org/apache/beam/sdk/io/jdbc/JdbcReadSchemaTransformProvider.java:
##########
@@ -164,12 +194,17 @@ public abstract static class JdbcReadSchemaTransformConfiguration implements Ser
public abstract String getDriverJars();
public void validate() throws IllegalArgumentException {
- if (Strings.isNullOrEmpty(getDriverClassName())) {
- throw new IllegalArgumentException("JDBC Driver class name cannot be blank.");
+ if (Strings.isNullOrEmpty(getDriverClassName()) && Strings.isNullOrEmpty(getJdbcType())) {
Review Comment:
IMHO, it's preferable to specify exactly one of the two, otherwise it's an error.
--
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.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [YAML] Add ability to specify JDBC type to read/write [beam]
Posted by "Polber (via GitHub)" <gi...@apache.org>.
Polber commented on code in PR #30024:
URL: https://github.com/apache/beam/pull/30024#discussion_r1470203741
##########
sdks/java/io/jdbc/src/main/java/org/apache/beam/sdk/io/jdbc/JdbcReadSchemaTransformProvider.java:
##########
@@ -164,12 +194,17 @@ public abstract static class JdbcReadSchemaTransformConfiguration implements Ser
public abstract String getDriverJars();
public void validate() throws IllegalArgumentException {
- if (Strings.isNullOrEmpty(getDriverClassName())) {
- throw new IllegalArgumentException("JDBC Driver class name cannot be blank.");
+ if (Strings.isNullOrEmpty(getDriverClassName()) && Strings.isNullOrEmpty(getJdbcType())) {
Review Comment:
Hmm that's a good point. It is redundant to specify both - specifying `jdbcType` that conflicts with `driverClassName` won't affect the code path which will uses the `driverClassName` parameter and ignores the `jdbcType`. This just might be misleading behavior, but I can remove the error for now since it isn't technically an error
--
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.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [YAML] Add ability to specify JDBC type to read/write [beam]
Posted by "robertwb (via GitHub)" <gi...@apache.org>.
robertwb commented on code in PR #30024:
URL: https://github.com/apache/beam/pull/30024#discussion_r1468257406
##########
sdks/java/io/jdbc/src/main/java/org/apache/beam/sdk/io/jdbc/JdbcReadSchemaTransformProvider.java:
##########
@@ -164,12 +194,17 @@ public abstract static class JdbcReadSchemaTransformConfiguration implements Ser
public abstract String getDriverJars();
public void validate() throws IllegalArgumentException {
- if (Strings.isNullOrEmpty(getDriverClassName())) {
- throw new IllegalArgumentException("JDBC Driver class name cannot be blank.");
+ if (Strings.isNullOrEmpty(getDriverClassName()) && Strings.isNullOrEmpty(getJdbcType())) {
Review Comment:
Is it an error to specify both?
##########
sdks/java/io/jdbc/src/main/java/org/apache/beam/sdk/io/jdbc/JdbcWriteSchemaTransformProvider.java:
##########
@@ -73,8 +78,29 @@ public JdbcWriteSchemaTransform(JdbcWriteSchemaTransformConfiguration config) {
}
protected JdbcIO.DataSourceConfiguration dataSourceConfiguration() {
+ String driverClassName = config.getDriverClassName();
+
+ if (Strings.isNullOrEmpty(driverClassName)) {
Review Comment:
Same as above, put these in a map. (Seems like it's the same map. Refactor to a common location?)
--
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.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [YAML] Add ability to specify JDBC type to read/write [beam]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #30024:
URL: https://github.com/apache/beam/pull/30024#issuecomment-1894551451
Assigning reviewers. If you would like to opt out of this review, comment `assign to next reviewer`:
R: @liferoad for label python.
R: @damondouglas for label java.
R: @damondouglas for label io.
Available commands:
- `stop reviewer notifications` - opt out of the automated review tooling
- `remind me after tests pass` - tag the comment author after tests pass
- `waiting on author` - shift the attention set back to the author (any comment or push by the author will return the attention set to the reviewers)
The PR bot will only process comments in the main thread (not review comments).
--
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.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [YAML] Add ability to specify JDBC type to read/write [beam]
Posted by "Polber (via GitHub)" <gi...@apache.org>.
Polber commented on code in PR #30024:
URL: https://github.com/apache/beam/pull/30024#discussion_r1473396971
##########
sdks/java/io/jdbc/src/main/java/org/apache/beam/sdk/io/jdbc/JdbcWriteSchemaTransformProvider.java:
##########
@@ -73,8 +78,29 @@ public JdbcWriteSchemaTransform(JdbcWriteSchemaTransformConfiguration config) {
}
protected JdbcIO.DataSourceConfiguration dataSourceConfiguration() {
+ String driverClassName = config.getDriverClassName();
+
+ if (Strings.isNullOrEmpty(driverClassName)) {
Review Comment:
Done.
--
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.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [YAML] Add ability to specify JDBC type to read/write [beam]
Posted by "robertwb (via GitHub)" <gi...@apache.org>.
robertwb merged PR #30024:
URL: https://github.com/apache/beam/pull/30024
--
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.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org