Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

all combined: adding Oracle database, --sqlFile feature #24

Closed
wants to merge 3 commits into from
Closed

all combined: adding Oracle database, --sqlFile feature #24

wants to merge 3 commits into from

Conversation

hilliao
Copy link

@hilliao hilliao commented May 22, 2018

I added 3 features in 3 commits. plus, I modified the Readme.md file which fixes the issues I raised. The changes require having a local maven repository with ojdbc8.jar dependency. I put the command to add that in my forked DBeam's Readme.md Building with ojdbc8.jar:
mvn install:install-file -Dfile=/usr/local/lib/ojdbc8.jar \ -DgroupId=com.oracle -DartifactId=ojdbc8 -Dversion=12.2.0.1 -Dpackaging=jar -DgeneratePom=true
I intend to add more unit tests for the added features but want to get your feedback on my code first. I'm hoping it will create value to your project and the open source community.

@codecov-io
Copy link

codecov-io commented May 22, 2018

Codecov Report

Merging #24 into master will decrease coverage by 4.42%.
The diff coverage is 60.46%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master     #24      +/-   ##
=========================================
- Coverage   88.23%   83.8%   -4.43%     
=========================================
  Files           6       6              
  Lines         221     247      +26     
  Branches       30      37       +7     
=========================================
+ Hits          195     207      +12     
- Misses         26      40      +14
Impacted Files Coverage Δ
...m/spotify/dbeam/options/DBeamPipelineOptions.scala 100% <ø> (ø) ⬆️
...ala/com/spotify/dbeam/options/JdbcExportArgs.scala 76.78% <42.1%> (-13.92%) ⬇️
...com/spotify/dbeam/options/JdbcConnectionArgs.scala 88.88% <62.5%> (-11.12%) ⬇️
.../scala/com/spotify/dbeam/JdbcAvroConversions.scala 82.5% <66.66%> (-0.62%) ⬇️
...src/main/scala/com/spotify/dbeam/JdbcAvroJob.scala 98.11% <90%> (-1.89%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update bd982f7...b809aa0. Read the comment docs.

@labianchin
Copy link
Collaborator

Can we split into separate PRs?

Build is failing, can you look into that?

As for oracle support: I wonder how would we make CI and release (currently manual) work with an outside JAR (that requires acceptance of the OTN license: http://www.oracle.com/technetwork/licenses/distribution-license-152002.html). Do you have any idea? Maybe we could try to not include oracle here but make it possible to combine both oracle and dbeam-core jars.

@hilliao
Copy link
Author

hilliao commented May 26, 2018

  • It's hard to split into 3 pull requests based on a branch starting at bd982f7. Oracle feature's commit edits 1 or 2 files in new arguments' commit. I did the hard work and split them into 3 pull requests but merging them in the following order will cause conflicts at step 2:
  1. new arguments
  2. oracle
  3. Readme doc improvement has change of 1 file: README.md

I suggest you review each pull requests and do additional testing per your needs. I highly recommend merging this pull request 24. If you really need to merge the 3 pull requests, make sure the final result is the same as merging this pull request, especially during conflict resolution. I pushed commits that fixed the failed unit tests in all 4 pull request. No failures.

  • The Oracle support is optional. This pull request's README.md tells how to include ojdbc8.jar by setting environment variable ojdbc8_PATH to the path of downloaded ojdbc8.jar. Currently, it builds without Oracle support in Travis CI build. To make Travis CI build work with Oracle, simply copy ojdbc8.jar to somewhere writeable on the Travis CI build server and set environment variable:
    export ojdbc8_PATH=/tmp/ojdbc8.jar
    Without setting that environment variable above, DBeam builds without Oracle support.

…oud Dataflow runner, build with Oracle ojdbc8.jar, a DockerFile for running DBeam.
@hilliao hilliao changed the title adding Oracle database, --sqlFile feature all combined: adding Oracle database, --sqlFile feature Jun 5, 2018
@hilliao
Copy link
Author

hilliao commented Jun 12, 2018

I found that your last commit was 20 days ago. This is not an actively maintained project. I don't intend to modify my pull requests or put more hours into it since I finished the project that needed DBeam. This is the most useful combined pull request. If you find something useful, best place to merge them is from here.

@labianchin
Copy link
Collaborator

I found that your last commit was 20 days ago. This is not an actively maintained project.

I am sorry to hear that.
DBeam is a stable project that we use in production. Still, I got other commitments that unfortunately made me an unresponsive maintainer.

I don't intend to modify my pull requests or put more hours into it since I finished the project that needed DBeam. This is the most useful combined pull request. If you find something useful, best place to merge them is from here.

I understand. I do find value on the readme and parameters improvements (#29 ). I will leave those open for us to work on them.

@labianchin labianchin closed this Jun 18, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

readme needs improvement
3 participants