-
Notifications
You must be signed in to change notification settings - Fork 146
APEXMALHAR-2462 Move JDBC related examples from datatorrent examples to apex-malhar examples #606
Conversation
94a2b79 to
f47bf72
Compare
|
@tweise @amberarrow @ashwinchandrap i fixed issues please review |
|
@tweise can you see |
f47bf72 to
bcc2305
Compare
bcc2305 to
e196d28
Compare
|
@tweise I updated the PR title and commit message. Please see |
|
Looks good, I'll merge in a couple of days if there are no further comments. |
examples/jdbc/pom.xml
Outdated
| <dependency> | ||
| <groupId>mysql</groupId> | ||
| <artifactId>mysql-connector-java</artifactId> | ||
| <version>5.1.36</version> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was already a discussion that the mysql connector should not be used due to licensing issue. Why is it required?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The original example is using the mysql jdbc driver and this dependency was probably included so that the apa is complete and can run. Would it be a problem given that example typically is used directly and not as a further dependency to another project.
What would be a good alternative? Make it provided? Use an alternative jdbc driver?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See https://issues.apache.org/jira/browse/APEXMALHAR-2461 - that JIRA needs to be addressed soon and before the next release.
|
|
||
| <!-- JDBC driver in use --> | ||
| <property> | ||
| <name>dt.application.JdbcToJdbcApp.operator.JdbcInput.prop.store.databaseDriver |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
application.xxx qualification is not needed here.
|
https://raw.githubusercontent.com/apache/apex-malhar/master/docs/operators/jdbcPollInputOperator.md contains links to old example location. This is an issue with previously moved examples also. I'm OK fixing this outside of this PR if it is addressed soon and before moving any further examples. |
examples/jdbc/.gitignore
Outdated
| @@ -0,0 +1 @@ | |||
| /target/ | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
isn't this taken care of by top level .gitignore ?
examples/jdbc/README.md
Outdated
| shell> mvn clean install | ||
|
|
||
| Upload the `target/jdbcInput-1.0-SNAPSHOT.apa` to the UI console if available or launch it from | ||
| the commandline using `apexcli`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
apexcli ?
examples/jdbc/README.md
Outdated
| Step 3: Build the code, | ||
| shell> mvn clean install | ||
|
|
||
| Upload the target/jdbcInput-1.0-SNAPSHOT.apa to the gateway |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
correct name of the .apa file, remove gateway reference
0d6150c to
4f90706
Compare
4f90706 to
e8cc278
Compare
|
@tweise addressed the comments. |
|
Looks like jdbc/README.md still has a reference to gateway (line 175) and the line following it (Step 4) is not meaningful, as it currently stands, for users launching from the command line. The mysql connector reference is still present; how about we address that in a later PR ? One option might be to check if MariaDB connector works as a drop-in replacement and use that if possible. It has LGPL Rest looks good. |
e8cc278 to
60e01c5
Compare
|
@amberarrow @tweise @devtagare please see |
9d50bd9 to
e7d4496
Compare
examples/jdbc/README.md
Outdated
| Step 3: Build the code, | ||
| shell> mvn clean install | ||
|
|
||
| Upload the target/malhar-examples-JDBC-3.8.0-SNAPSHOT.apa to the gateway |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
still to be addressed
|
@prasannapramod unless the review comments are addressed, there is no need to ask for another review. My take is that we should avoid further code additions to Malhar unless the maintenance situation is clear and folks step up to fix issues as they arise (CI errors, dependency issues etc.). |
examples/jdbc/pom.xml
Outdated
| <artifactId>malhar-examples-JDBC</artifactId> | ||
| <packaging>jar</packaging> | ||
|
|
||
| <!-- change these to the appropriate values --> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove these lines
| } | ||
|
|
||
| /** | ||
| * This method can be modified to have field mappings based on used defined |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
spelling
e7d4496 to
fc234b1
Compare
|
@amberarrow @tweise Changed references of mysql in README.txt and property files to hsqldb removed "gateway" reference as well. @tweise Currently there are 3 active example related PR's #609, #604, #606 (current) for fileIO, JMS and Jdbc respectively. I will not rise new example PR's till this are merged and any pending issues you are referring are resolved. |
|
@prasannapramod there is follow-up work: package should have been examples.jdbc (please make sure that is consistent for all examples). There is some formatting issue in the README. And I believe there are still issues with user docs pointing to old example locations. Would be good to see pending issues addressed next. Are you going to take up maintenance for all of the example projects? |
@tweise @amberarrow @ashwinchandrap please review