You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@beam.apache.org by GitBox <gi...@apache.org> on 2020/07/30 14:18:18 UTC
[GitHub] [beam] piotr-szuberski opened a new pull request #12423: [BEAM-10135][BEAM-10136] Refactor jdbc external transform registrar
piotr-szuberski opened a new pull request #12423:
URL: https://github.com/apache/beam/pull/12423
During writing kinesis for cross-language it was suggested to emplace all classes connected to cross-language in one file. I do the same here for jdbc.
R: @chamikaramj
------------------------
Thank you for your contribution! Follow this checklist to help us incorporate your contribution quickly and easily:
- [ ] [**Choose reviewer(s)**](https://beam.apache.org/contribute/#make-your-change) and mention them in a comment (`R: @username`).
- [ ] Format the pull request title like `[BEAM-XXX] Fixes bug in ApproximateQuantiles`, where you replace `BEAM-XXX` with the appropriate JIRA issue, if applicable. This will automatically link the pull request to the issue.
- [ ] 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://beam.apache.org/contribute/#make-reviewers-job-easier).
Post-Commit Tests Status (on master branch)
------------------------------------------------------------------------------------------------
Lang | SDK | Dataflow | Flink | Samza | Spark | Twister2
--- | --- | --- | --- | --- | --- | ---
Go | [](https://ci-beam.apache.org/job/beam_PostCommit_Go/lastCompletedBuild/) | --- | [](https://ci-beam.apache.org/job/beam_PostCommit_Go_VR_Flink/lastCompletedBuild/) | --- | [](https://ci-beam.apache.org/job/beam_PostCommit_Go_VR_Spark/lastCompletedBuild/) | ---
Java | [](https://ci-beam.apache.org/job/beam_PostCommit_Java/lastCompletedBuild/) | [](https://ci-beam.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Dataflow/lastCompletedBuild/)<br>[](https://ci-beam.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Dataflow_Java11/lastCompletedBuild/) | [](https://ci-beam.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Flink/lastCompletedBuild/)<br>[](https://ci-beam.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Flink_Java11/lastCompletedBuild/)<br>[](https://ci-beam.apache.org/job/beam_PostCommit_Java_PVR_Flink_Batch/lastCompletedBuild/)<br>[](https://ci-beam.apache.org/job/beam_PostCommit_Java_PVR_Flink_Streaming/lastCompletedBuild/) | [](https://ci-beam.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Samza/lastCompletedBuild/) | [](https://ci-beam.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Spark/lastCompletedBuild/)<br>[](https://ci-beam.apache.org/job/beam_PostCommit_Java_PVR_Spark_Batch/lastCompletedBuild/)<br>[](https://ci-beam.apache.org/job/beam_PostCommit_Java_ValidatesRunner_SparkStructuredStreaming/lastCompletedBuild/) | [](https://builds.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Twister2/lastCompletedBuild/)
Python | [](https://ci-beam.apache.org/job/beam_PostCommit_Python2/lastCompletedBuild/)<br>[](https://ci-beam.apache.org/job/beam_PostCommit_Python35/lastCompletedBuild/)<br>[](https://ci-beam.apache.org/job/beam_PostCommit_Python36/lastCompletedBuild/)<br>[](https://ci-beam.apache.org/job/beam_PostCommit_Python37/lastCompletedBuild/)<br>[](https://ci-beam.apache.org/job/beam_PostCommit_Python38/lastCompletedBuild/) | [](https://ci-beam.apache.org/job/beam_PostCommit_Py_VR_Dataflow/lastCompletedBuild/)<br>[](https://ci-beam.apache.org/job/beam_PostCommit_Py_VR_Dataflow_V2/lastCompletedBuild/)<br>[](https://ci-beam.apache.org/job/beam_PostCommit_Py_ValCont/lastCompletedBuild/) | [](https://ci-beam.apache.org/job/beam_PreCommit_Python2_PVR_Flink_Cron/lastCompletedBuild/)<br>[](https://ci-beam.apache.org/job/beam_PostCommit_Python35_VR_Flink/lastCompletedBuild/) | --- | [](https://ci-beam.apache.org/job/beam_PostCommit_Python_VR_Spark/lastCompletedBuild/) | ---
XLang | [](https://ci-beam.apache.org/job/beam_PostCommit_XVR_Direct/lastCompletedBuild/) | --- | [](https://ci-beam.apache.org/job/beam_PostCommit_XVR_Flink/lastCompletedBuild/) | --- | [](https://ci-beam.apache.org/job/beam_PostCommit_XVR_Spark/lastCompletedBuild/) | ---
Pre-Commit Tests Status (on master branch)
------------------------------------------------------------------------------------------------
--- |Java | Python | Go | Website
--- | --- | --- | --- | ---
Non-portable | [](https://ci-beam.apache.org/job/beam_PreCommit_Java_Cron/lastCompletedBuild/) | [](https://ci-beam.apache.org/job/beam_PreCommit_Python_Cron/lastCompletedBuild/)<br>[](https://ci-beam.apache.org/job/beam_PreCommit_PythonLint_Cron/lastCompletedBuild/)<br>[](https://ci-beam.apache.org/job/beam_PreCommit_PythonDocker_Cron/lastCompletedBuild/) | [](https://ci-beam.apache.org/job/beam_PreCommit_Go_Cron/lastCompletedBuild/) | [](https://ci-beam.apache.org/job/beam_PreCommit_Website_Cron/lastCompletedBuild/)
Portable | --- | [](https://ci-beam.apache.org/job/beam_PreCommit_Portable_Python_Cron/lastCompletedBuild/) | --- | ---
See [.test-infra/jenkins/README](https://github.com/apache/beam/blob/master/.test-infra/jenkins/README.md) for trigger phrase, status and link of all Jenkins jobs.
----------------------------------------------------------------
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] [beam] piotr-szuberski edited a comment on pull request #12423: [BEAM-10135][BEAM-10136] Refactor jdbc external transform registrar
Posted by GitBox <gi...@apache.org>.
piotr-szuberski edited a comment on pull request #12423:
URL: https://github.com/apache/beam/pull/12423#issuecomment-667916022
> "During writing kinesis for cross-language it was suggested to emplace all classes connected to cross-language in one file"
>
> I think Java generally recommends keeping classes in separate files, doesn't it ? I think either way should be fine as long as x-lang framework is concerned.
@chamikaramj In general yes, but most of beam connectors have huge files with nested classes instead of splitting them to separate packages and small files. In the case of external transforms it's a bit more elegant because ExternalTransformRegistrar has to override function knownTransforms() which assumes there can be more than one transform under it.
But I agree that it's not that necessary to put everything into one file, so if you prefer it to be like it is now then I'll close this PR.
----------------------------------------------------------------
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] [beam] piotr-szuberski edited a comment on pull request #12423: [BEAM-10135][BEAM-10136] Refactor jdbc external transform registrar
Posted by GitBox <gi...@apache.org>.
piotr-szuberski edited a comment on pull request #12423:
URL: https://github.com/apache/beam/pull/12423#issuecomment-667916022
> "During writing kinesis for cross-language it was suggested to emplace all classes connected to cross-language in one file"
>
> I think Java generally recommends keeping classes in separate files, doesn't it ? I think either way should be fine as long as x-lang framework is concerned.
@chamikaramj In general yes, but most of beam connectors have huge files with nested classes instead of splitting them to separate packages and small files. In the case of external transforms I think it's a bit more elegant because ExternalTransformRegistrar has to override function knownTransforms() which assumes there can be more than one transform under it.
But I agree that it's not that necessary to put everything into one file, so if you prefer it to be like it is now then I'll close this PR.
----------------------------------------------------------------
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] [beam] chamikaramj commented on pull request #12423: [BEAM-10135][BEAM-10136] Refactor jdbc external transform registrar
Posted by GitBox <gi...@apache.org>.
chamikaramj commented on pull request #12423:
URL: https://github.com/apache/beam/pull/12423#issuecomment-670157940
We can merge this.
BTW I'm curious, have you tested the jdbc.py against Dataflow ? If not can you try that ?
Instructions for running x-lang wrappers against Dataflow are here - https://github.com/apache/beam/tree/master/sdks/python/apache_beam/examples/kafkataxi
----------------------------------------------------------------
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] [beam] piotr-szuberski commented on pull request #12423: [BEAM-10135][BEAM-10136] Refactor jdbc external transform registrar
Posted by GitBox <gi...@apache.org>.
piotr-szuberski commented on pull request #12423:
URL: https://github.com/apache/beam/pull/12423#issuecomment-667916022
> "During writing kinesis for cross-language it was suggested to emplace all classes connected to cross-language in one file"
>
> I think Java generally recommends keeping classes in separate files, doesn't it ? I think either way should be fine as long as x-lang framework is concerned.
@chamikaramj In general yes, but most of beam connectors have huge files with nested classes instead of splitting them to separate packages and small files. In the case of external transforms it's a bit more elegant because ExternalTransformRegistrar has to override function knownTransforms() which assumes there can be more than one transform under it. The builders actually should also be in separate files from the registrar.
But I agree that it's not that necessary to put everything into one file, so if you prefer it to be like it is now then I'll close this PR.
----------------------------------------------------------------
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] [beam] piotr-szuberski commented on pull request #12423: [BEAM-10135][BEAM-10136] Refactor jdbc external transform registrar
Posted by GitBox <gi...@apache.org>.
piotr-szuberski commented on pull request #12423:
URL: https://github.com/apache/beam/pull/12423#issuecomment-671210989
I'm not sure whether the e-mail notifications are sent without mentioning so just to be on the safe side that you get one: @chamikaramj
----------------------------------------------------------------
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] [beam] piotr-szuberski commented on pull request #12423: [BEAM-10135][BEAM-10136] Refactor jdbc external transform registrar
Posted by GitBox <gi...@apache.org>.
piotr-szuberski commented on pull request #12423:
URL: https://github.com/apache/beam/pull/12423#issuecomment-671210285
> BTW I'm curious, have you tested the jdbc.py against Dataflow ? If not can you try that ?
> Instructions for running x-lang wrappers against Dataflow are here - https://github.com/apache/beam/tree/master/sdks/python/apache_beam/examples/kafkataxi
I tried to run this test using the setup from kafkataxi but I wasn't successful. Now when I think about that it could not be possible to use testcontainers with dataflow. I'll try it on bigquery or a tunneled postgres this week.
----------------------------------------------------------------
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] [beam] chamikaramj commented on pull request #12423: [BEAM-10135][BEAM-10136] Refactor jdbc external transform registrar
Posted by GitBox <gi...@apache.org>.
chamikaramj commented on pull request #12423:
URL: https://github.com/apache/beam/pull/12423#issuecomment-667303832
"During writing kinesis for cross-language it was suggested to emplace all classes connected to cross-language in one file"
I think Java generally recommends keeping classes in separate files, doesn't it ? I think either way should be fine as long as x-lang framework is concerned.
----------------------------------------------------------------
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] [beam] chamikaramj commented on pull request #12423: [BEAM-10135][BEAM-10136] Refactor jdbc external transform registrar
Posted by GitBox <gi...@apache.org>.
chamikaramj commented on pull request #12423:
URL: https://github.com/apache/beam/pull/12423#issuecomment-671416413
Was there an error ? Could you file a JIRA ?
----------------------------------------------------------------
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] [beam] piotr-szuberski commented on pull request #12423: [BEAM-10135][BEAM-10136] Refactor jdbc external transform registrar
Posted by GitBox <gi...@apache.org>.
piotr-szuberski commented on pull request #12423:
URL: https://github.com/apache/beam/pull/12423#issuecomment-666395401
Run Python 3.8 PostCommit
----------------------------------------------------------------
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] [beam] chamikaramj merged pull request #12423: [BEAM-10135][BEAM-10136] Refactor jdbc external transform registrar
Posted by GitBox <gi...@apache.org>.
chamikaramj merged pull request #12423:
URL: https://github.com/apache/beam/pull/12423
----------------------------------------------------------------
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] [beam] piotr-szuberski commented on pull request #12423: [BEAM-10135][BEAM-10136] Refactor jdbc external transform registrar
Posted by GitBox <gi...@apache.org>.
piotr-szuberski commented on pull request #12423:
URL: https://github.com/apache/beam/pull/12423#issuecomment-671417576
@chamikaramj I'll try to run it again this week just to be sure. If I'm not successful then of course I'll create an issue.
----------------------------------------------------------------
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