JDBC Writer #893

jinhyukchang · 2016-03-29T18:36:00Z

Pull request for JDBC Writer. Will combine to one commit after the review.

Design doc: https://docs.google.com/a/linkedin.com/document/d/1pSWZtgNssxaV1c19xdd1rKft9JaXM2qmqDwwbIb0scM/edit?usp=sharing

coveralls · 2016-03-29T18:53:26Z

Coverage increased (+0.7%) to 42.527% when pulling 19e7dbd on jinhyukchang:master into f5573e1 on linkedin:master.

kadaan · 2016-03-29T19:54:12Z

Any reason the design doc can't be public?

coveralls · 2016-03-30T22:11:03Z

Coverage increased (+1.9%) to 43.763% when pulling f4e408a on jinhyukchang:master into f5573e1 on linkedin:master.

coveralls · 2016-03-31T22:18:42Z

Coverage increased (+1.9%) to 43.684% when pulling ac1de18 on jinhyukchang:master into f5573e1 on linkedin:master.

chavdar · 2016-04-15T19:08:43Z

@kadaan Internally, for reviews, we use a company google docs account. Once we finalize the internal review, we'll share the design.

chavdar · 2016-04-15T19:21:27Z

gobblin-api/src/main/java/gobblin/configuration/ConfigurationKeys.java

@@ -102,6 +102,7 @@
  public static final String JOB_JAR_FILES_KEY = "job.jars";
  public static final String JOB_LOCAL_FILES_KEY = "job.local.files";
  public static final String JOB_HDFS_FILES_KEY = "job.hdfs.files";
+  public static final String JOB_JAR_HDFS_FILES_KEY = "job.jars.hdfs";


Please make name consistent with above property: job.hdfs.jars.

…omment

sahilTakiar · 2016-04-25T21:00:00Z

gobblin-core/src/main/java/gobblin/converter/filter/AvroFieldsPickConverter.java

+   * @see gobblin.converter.AvroToAvroConverterBase#convertSchema(org.apache.avro.Schema, gobblin.configuration.WorkUnitState)
+   */
+  @Override
+  public Schema convertSchema(Schema inputSchema, WorkUnitState workUnit) throws SchemaConversionException {


Why does this method need to first populate a list of fields to remove, and then remove them remove them each from the schema? Can't it simply create a Schema based on the value of CONVERTER_AVRO_FIELDS_PICK_FIELDS.

AvroUtils.getFieldSchema will fetch the Schema of a field given a dot separated path to the field name.

It was just to reduce it to remove problem as removal tool is already there in Gobblin. It seems like it can be easily done by AvroUtils and will update it.

I was exploring AvroUtils and was not able to find an easy way to reconstruct Schema mainly because getFieldSchema only provides leaf information which means anyway converter needs to traverse the Schema.
I've used Trie to present dependencies from input and reconstruct the Schema. (Just traversing Schema and add field when it matches was dropped once I learned Schema fields are immutable). Let me know what you think.

I see, yes its unfortunate that just having the leaf Schemas is not enough. A Trie is one way to do this, but I think any nested data format would work. For example, I think you could do this using JSON, which has the advantage that you can use standard libraries like GSON. However, your current Trie approach works too, so I will leave it up to you.

sahilTakiar · 2016-04-26T02:31:52Z

gobblin-utility/src/main/java/gobblin/util/jdbc/DataSourceBuilder.java

+
+  private void validateNotEmpty(String s, String name) {
+    if(StringUtils.isEmpty(s)) {
+      throw new IllegalArgumentException(name + " should not be empty.");


You can use Guava's Preconditions to do this

…of removing fields, Using Lombok, added converterInitializer to retrieve date field info once

jinhyukchang · 2016-04-29T17:53:09Z

Update the code per PR comments.

Conflicts: build.gradle gobblin-core/src/main/java/gobblin/converter/filter/AvroSchemaFieldRemover.java gobblin-runtime/src/main/java/gobblin/runtime/mapreduce/MRJobLauncher.java

jinhyukchang · 2016-04-29T18:54:21Z

Merged from origin to sync up to date.

coveralls · 2016-04-29T20:15:12Z

Coverage increased (+0.9%) to 45.179% when pulling 67598da on jinhyukchang:master into 911ab2e on linkedin:master.

sahilTakiar · 2016-05-02T21:38:20Z

...in-core/src/main/java/gobblin/converter/initializer/AvroToJdbcEntryConverterInitializer.java

+import gobblin.writer.commands.JdbcWriterCommands;
+import gobblin.writer.commands.JdbcWriterCommandsFactory;
+
+public class AvroToJdbcEntryConverterInitializer implements ConverterInitializer {


javadoc please

jinhyukchang · 2016-05-03T19:19:30Z

@sahilTakiar Just updated the code addressing PR. Would you take a look when you have a chance?

coveralls · 2016-05-03T19:35:16Z

Coverage increased (+0.7%) to 44.991% when pulling 09ffbfc on jinhyukchang:master into 911ab2e on linkedin:master.

sahilTakiar · 2016-05-09T18:30:48Z

hey @jinhyukchang I discussed with @chavdar and everything in this PR looks good! I can merge this PR, but I have one last request.

Can you add some documentation to the Gobblin Docs about this feature, you can probably just copy what you have in your Google Doc. If you have time its always nice to add documentation on configuration keys and documentation on the different plugin points. This is generally useful for any new Gobblin user who wants to write to a RDMS (I already know of one user waiting on this PR because they want to use this).

All Gobblin documentation is checked into GitHub under gobblin-docs. I suggest putting the documentation in a new file called gobblin-docs/user-guide/Gobblin-JDBC-Writer.md. The format of the files is all in Markdown. It's should be a relatively simple process, but if you have more questions this page has more details: http://gobblin.readthedocs.io/en/latest/developer-guide/Contributing/

chavdar · 2016-05-09T18:48:26Z

@jinhyukchang we will feature the JDBC writer as one of the highlights for our next release (0.7.0) so it will be great to have some supporting documentation.

jinhyukchang · 2016-05-09T20:13:34Z

@sahilTakiar , @chavdar ,

Great, thanks for the review and will add documentation shortly.

jinhyukchang · 2016-05-10T18:25:09Z

@sahilTakiar , @chavdar
Just added documentation. Let me know how it looks. (http://jnchang-ld1:8000/)

Seems now there's a conflict with master branch and I will resolve it once documentation is reviewed.

sahilTakiar · 2016-05-10T19:57:11Z

Documentation LGTM!

When you rebase this PR, can you also squash all the commits together into one commit?

jinhyukchang · 2016-05-10T21:04:10Z

Sure, will do that.

Update code following findbugs report. Conflicts: gobblin-runtime/src/main/java/gobblin/runtime/AbstractJobLauncher.java

coveralls · 2016-05-10T23:47:01Z

Coverage increased (+0.5%) to 45.648% when pulling 387da5a on jinhyukchang:master into 9971e73 on linkedin:master.

coveralls · 2016-05-11T19:24:01Z

Coverage increased (+0.7%) to 45.968% when pulling 43d40e0 on jinhyukchang:master into 73e8720 on linkedin:master.

jinhyukchang added 9 commits March 9, 2016 10:13

Initial checkin for JDBC writer

c471810

Added Converters for JDBC writer and WriterInitializer

a7a9386

Added comment and test cases

ef8339d

Added test cases

3e53ba0

Merged latest code and resolved conflicts

64ca02a

Added comments

5082867

Merge remote-tracking branch 'gobblin_mainline/master'

cd120cf

Merge remote-tracking branch 'gobblin_mainline/master'

9f92a35

Updated test case to use resource instead of file path

19e7dbd

Make it Java 7 compatible

f4e408a

encrypt key param for JDBC Writer, Job jars for the file in HDFS

ac1de18

chavdar reviewed Apr 15, 2016
View reviewed changes

jinhyukchang added 2 commits April 19, 2016 10:30

AvroToJdbcEntryConverter to take long type for date in millisecond.

771ae10

Moved jdbc publisher config keys into JdbcPublisher class as per PR c…

9d4d137

…omment

sahilTakiar reviewed Apr 25, 2016
View reviewed changes

sahilTakiar reviewed Apr 26, 2016
View reviewed changes

Updated comments, AvroFieldsPickConverter to pick the fields instead …

726acca

…of removing fields, Using Lombok, added converterInitializer to retrieve date field info once

Merge branch 'master' of https://github.com/linkedin/gobblin

67598da

Conflicts: build.gradle gobblin-core/src/main/java/gobblin/converter/filter/AvroSchemaFieldRemover.java gobblin-runtime/src/main/java/gobblin/runtime/mapreduce/MRJobLauncher.java

sahilTakiar reviewed May 2, 2016
View reviewed changes

Addressed PR comments

09ffbfc

Added documentation on JDBC Writer

cf89cef

Merge branch 'master' of https://github.com/linkedin/gobblin

387da5a

Update code following findbugs report. Conflicts: gobblin-runtime/src/main/java/gobblin/runtime/AbstractJobLauncher.java

Merge branch 'master' of https://github.com/linkedin/gobblin

43d40e0

sahilTakiar merged commit 98893b4 into apache:master May 12, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

JDBC Writer #893

JDBC Writer #893

jinhyukchang commented Mar 29, 2016

coveralls commented Mar 29, 2016

kadaan commented Mar 29, 2016

coveralls commented Mar 30, 2016

coveralls commented Mar 31, 2016

chavdar commented Apr 15, 2016

chavdar Apr 15, 2016

jinhyukchang Apr 19, 2016

sahilTakiar Apr 25, 2016

jinhyukchang Apr 26, 2016

jinhyukchang Apr 29, 2016

sahilTakiar May 2, 2016

sahilTakiar Apr 26, 2016

jinhyukchang Apr 26, 2016

jinhyukchang commented Apr 29, 2016 •

edited

jinhyukchang commented Apr 29, 2016

coveralls commented Apr 29, 2016 •

edited

sahilTakiar May 2, 2016

jinhyukchang May 3, 2016

jinhyukchang commented May 3, 2016

coveralls commented May 3, 2016 •

edited

sahilTakiar commented May 9, 2016

chavdar commented May 9, 2016

jinhyukchang commented May 9, 2016

jinhyukchang commented May 10, 2016 •

edited

sahilTakiar commented May 10, 2016

jinhyukchang commented May 10, 2016

coveralls commented May 10, 2016 •

edited

coveralls commented May 11, 2016 •

edited

JDBC Writer #893

JDBC Writer #893

Conversation

jinhyukchang commented Mar 29, 2016

coveralls commented Mar 29, 2016

kadaan commented Mar 29, 2016

coveralls commented Mar 30, 2016

coveralls commented Mar 31, 2016

chavdar commented Apr 15, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jinhyukchang commented Apr 29, 2016 • edited

jinhyukchang commented Apr 29, 2016

coveralls commented Apr 29, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jinhyukchang commented May 3, 2016

coveralls commented May 3, 2016 • edited

sahilTakiar commented May 9, 2016

chavdar commented May 9, 2016

jinhyukchang commented May 9, 2016

jinhyukchang commented May 10, 2016 • edited

sahilTakiar commented May 10, 2016

jinhyukchang commented May 10, 2016

coveralls commented May 10, 2016 • edited

coveralls commented May 11, 2016 • edited

jinhyukchang commented Apr 29, 2016 •

edited

coveralls commented Apr 29, 2016 •

edited

coveralls commented May 3, 2016 •

edited

jinhyukchang commented May 10, 2016 •

edited

coveralls commented May 10, 2016 •

edited

coveralls commented May 11, 2016 •

edited