APEXMALHAR-2116 Added FS record reader operator, module, test #326

yogidevendra · 2016-06-21T14:38:39Z

No description provided.

yogidevendra · 2016-06-21T14:40:33Z

@amberarrow Could you please review this?

amberarrow · 2016-06-21T14:57:21Z

Ok, I'll take a look

amberarrow · 2016-06-21T23:08:11Z

library/src/main/java/org/apache/apex/malhar/lib/fs/FSRecordReader.java

+import com.datatorrent.lib.io.block.ReaderContext;
+
+/**
+ * This operator can be used for reading records/tuples from Filesystem 


This line and several others have trailing whitespace; this is generally considered not a good thing -- see
for instance: http://programmers.stackexchange.com/questions/121555/why-is-trailing-whitespace-a-big-deal
Suggest removing all trailing white space.

otterc · 2016-06-21T23:30:30Z

@yogidevendra
Can you please explain how the downstream operators will know which block a record belongs to?
Specifically when you converge, the information that which record belongs to which block will be lost.

Is this being created for any specific use case? Can you please share that?

yogidevendra · 2016-06-23T05:59:32Z

@ChandniSingh
This is mainly for the usecases where downstream operators do not care about which block the incoming record belongs to. For example, dedup, enrichment, projection, transform operator just work on POJO. They do not care about which file or which block does this tuple belong to.

Usecases, in this context do not intend to retain the original sequence of records at sources. They can consider each tuple to be independent and just focus on how to process them at scale. In this case, this module will be connected to "byte[] to POJO converter" (Say delimited parser) and then further to dedup or enrichment processing operator. Finally, output of dedup will be connected to some output operator.

yogidevendra · 2016-06-23T07:51:36Z

Incorporated review comments.

amberarrow · 2016-06-23T13:01:13Z

library/src/main/java/org/apache/apex/malhar/lib/fs/FSRecordReaderModule.java

+ * 4. recursive: if scan recursively input directories<br/>
+ * 5. blockSize: block size used to read input blocks of file<br/>
+ * 6. readersCount: count of readers to read input file<br/>
+ * 7. sequencialFileRead: If emit file blocks in sequence?<br/>


sequencial => sequential

amberarrow · 2016-06-24T04:24:30Z

Still looking at this -- probably take a couple of days more.

amberarrow · 2016-06-28T00:09:21Z

library/src/main/java/org/apache/apex/malhar/lib/fs/FSRecordReader.java

+     * Code for enabling BeanUtils to accept comma separated string to
+     * initialize FIELD_TYPE[]
+     */
+    class RecordReaderModeConverter extends AbstractConverter


Could you provide more detail on why this is necessary and how it is used ?

yogidevendra · 2016-06-29T16:00:55Z

@amberarrow Removed the unnecessary code for BeanUtils convertor. But, facing some issues with travis build. Build failures seems to be non-deterministic. (Different unit tests failed at multiple attempts).

Please let me know if you have any other suggestion regarding above code changes. I will update the PR for the same. We should keep this PR on hold till travis build is stable.

amberarrow · 2016-06-29T16:13:01Z

@yogidevendra OK, rest looks good, though I'd like to test it with a small sample application on the cluster; do you have such a sample ? It would also be a good addition to the examples collection.

yogidevendra · 2016-06-29T16:15:09Z

Let me know the location to add sample app. I will open separate PR for sample app in the respective repo.

amberarrow · 2016-06-29T16:27:40Z

https://github.com/DataTorrent/examples under tutorials

amberarrow · 2016-06-29T22:36:13Z

library/src/main/java/org/apache/apex/malhar/lib/fs/FSRecordReaderModule.java

+ * The module reads data in parallel, following parameters can be configured
+ * <br/>
+ * 1. files: list of file(s)/directories to read<br/>
+ * 2. filePatternRegularExp: Files names matching given regex will be read<br/>


Files names => Files with names

amberarrow · 2016-06-29T22:52:18Z

library/src/main/java/org/apache/apex/malhar/lib/fs/FSRecordReaderModule.java

+   */
+  public void setFiles(String files)
+  {
+    this.files = files;


Shouldn't the field be named "directories" if we expect it to be a list of directories ?

I tried to keep it consistent with FSInputModule.java. If you feel that readability/intuitive names are more important than having consistency across operators then I am fine with changing this field name.

DT-Priyanka · 2016-07-07T12:24:26Z

library/src/main/java/org/apache/apex/malhar/lib/fs/FSRecordReaderModule.java

+  /**
+   * Length for fixed width record
+   */
+  private int recordLength;


We should either have default value or mark Min constraint to make sure it is set by user.

yogidevendra · 2016-07-08T08:11:02Z

@DT-Priyanka Incorporated changes based on your review feedback.
@amberarrow Could you please do a final round of review and then merge if there no further comments from the community?

yogidevendra · 2016-07-12T03:46:42Z

@amberarrow Could you please merge this if there are no more comments.

yogidevendra · 2016-07-14T09:11:26Z

@DT-Priyanka Could you please merge this if there are no more comments?

DT-Priyanka · 2016-07-15T07:06:36Z

library/src/main/java/org/apache/apex/malhar/lib/fs/FSRecordReaderModule.java

+   * Length for fixed width record
+   */
+  @Min(1)
+  private int recordLength;


Record length will be optional in case of DELIMITED_RECORD and is compulsory in case of FIXED_LENGHT records. so keeping it either optional or compulsory field both have pros and cons.

Oh. Good point.
Should we separate this into two different classes. FixedWidthRecordReader, DelimitedRecordReader? That will make configuration clean.

@amberarrow Any thoughts?

Adding another class seems like overkill; how about removing the annotation and adding a check in the code to ensure that the value is positive if mode is FIXED_WIDTH_RECORD ?

@amberarrow As per your suggestion, removed @ min annotation for record length.
Added validation in the code to ensure that the value is positive if FIXED_WIDTH_RECORD.
Also, added test case for validating this.

Could you have a look and merge this if it looks OK.

amberarrow · 2016-07-25T01:59:42Z

There are a couple of minor issues:

The commit message is missing the JIRA (see instructions at https://apex.apache.org/contributing.html:
"Commit messages need to reference JIRA (pull requests will be linked to ticket)"
Several lines (all inside comments) have whitespace at the end; please remove.

2. javadoc improvements. 3. Adding default values 4. Incorporating review comments.

yogidevendra · 2016-07-25T15:41:10Z

@amberarrow Updated as per the feedback.

amberarrow reviewed Jun 21, 2016
View reviewed changes

yogidevendra force-pushed the APEXMALHAR-2116-record-reader branch from aaeb7fe to dd31bbe Compare June 23, 2016 07:00

amberarrow reviewed Jun 23, 2016
View reviewed changes

yogidevendra force-pushed the APEXMALHAR-2116-record-reader branch from dd31bbe to 75ef1d5 Compare June 24, 2016 08:16

amberarrow reviewed Jun 28, 2016
View reviewed changes

yogidevendra force-pushed the APEXMALHAR-2116-record-reader branch from 75ef1d5 to c4a6d55 Compare June 29, 2016 11:59

yogidevendra closed this Jun 29, 2016

yogidevendra reopened this Jun 29, 2016

yogidevendra force-pushed the APEXMALHAR-2116-record-reader branch from c4a6d55 to e5dba39 Compare June 29, 2016 12:47

yogidevendra closed this Jun 29, 2016

yogidevendra reopened this Jun 29, 2016

amberarrow reviewed Jun 29, 2016
View reviewed changes

yogidevendra force-pushed the APEXMALHAR-2116-record-reader branch from e5dba39 to 506ab58 Compare June 30, 2016 10:49

yogidevendra closed this Jul 4, 2016

yogidevendra reopened this Jul 4, 2016

yogidevendra mentioned this pull request Jul 4, 2016

SPOI-8561 Record reader example app dtpublic/examples#16

Merged

DT-Priyanka reviewed Jul 7, 2016
View reviewed changes

yogidevendra force-pushed the APEXMALHAR-2116-record-reader branch from 506ab58 to 5e955e4 Compare July 8, 2016 07:46

DT-Priyanka reviewed Jul 15, 2016
View reviewed changes

yogidevendra force-pushed the APEXMALHAR-2116-record-reader branch from 5e955e4 to ba8bbbe Compare July 21, 2016 11:46

APEXMALHAR-2116 : Added FS record reader operator, module, test

02d657c

2. javadoc improvements. 3. Adding default values 4. Incorporating review comments.

yogidevendra force-pushed the APEXMALHAR-2116-record-reader branch from ba8bbbe to 02d657c Compare July 25, 2016 06:37

asfgit merged commit 02d657c into apache:master Jul 31, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

APEXMALHAR-2116 Added FS record reader operator, module, test #326

APEXMALHAR-2116 Added FS record reader operator, module, test #326

yogidevendra commented Jun 21, 2016

yogidevendra commented Jun 21, 2016

amberarrow commented Jun 21, 2016

amberarrow Jun 21, 2016

otterc commented Jun 21, 2016

yogidevendra commented Jun 23, 2016

yogidevendra commented Jun 23, 2016

amberarrow Jun 23, 2016

amberarrow commented Jun 24, 2016

amberarrow Jun 28, 2016

yogidevendra commented Jun 29, 2016

amberarrow commented Jun 29, 2016

yogidevendra commented Jun 29, 2016

amberarrow commented Jun 29, 2016

amberarrow Jun 29, 2016

amberarrow Jun 29, 2016

yogidevendra Jun 30, 2016

DT-Priyanka Jul 7, 2016

yogidevendra commented Jul 8, 2016

yogidevendra commented Jul 12, 2016

yogidevendra commented Jul 14, 2016 •

edited

DT-Priyanka Jul 15, 2016

yogidevendra Jul 15, 2016

amberarrow Jul 15, 2016

yogidevendra Jul 21, 2016

amberarrow commented Jul 25, 2016

yogidevendra commented Jul 25, 2016

APEXMALHAR-2116 Added FS record reader operator, module, test #326

APEXMALHAR-2116 Added FS record reader operator, module, test #326

Conversation

yogidevendra commented Jun 21, 2016

yogidevendra commented Jun 21, 2016

amberarrow commented Jun 21, 2016

Choose a reason for hiding this comment

otterc commented Jun 21, 2016

yogidevendra commented Jun 23, 2016

yogidevendra commented Jun 23, 2016

Choose a reason for hiding this comment

amberarrow commented Jun 24, 2016

Choose a reason for hiding this comment

yogidevendra commented Jun 29, 2016

amberarrow commented Jun 29, 2016

yogidevendra commented Jun 29, 2016

amberarrow commented Jun 29, 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

yogidevendra commented Jul 8, 2016

yogidevendra commented Jul 12, 2016

yogidevendra commented Jul 14, 2016 • edited

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

amberarrow commented Jul 25, 2016

yogidevendra commented Jul 25, 2016

yogidevendra commented Jul 14, 2016 •

edited