You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@datafu.apache.org by Piyush Sharma <ps...@gmail.com> on 2016/12/31 10:47:25 UTC
Review Request 55110: DATAFU-106 Test files are currently created in
the
subdirectory folder (e.g. datafu-pig/input*). For better organization,
they should be created in a subdirectory.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55110/
-----------------------------------------------------------
Review request for DataFu.
Repository: datafu
Description
-------
DATAFU-106: Test files are currently created in the subdirectory folder (e.g. datafu-pig/input*). For better organization, they should be created in a subdirectory. This also makes it easier to exclude them all with gitignore.
(issue: https://issues.apache.org/jira/browse/DATAFU-106)
Diffs
-----
datafu-pig/src/test/java/datafu/test/pig/PigTests.java d1d6fcc
datafu-pig/src/test/java/datafu/test/pig/test_filesSubdir/TestFilesSubdirTest.java PRE-CREATION
Diff: https://reviews.apache.org/r/55110/diff/
Testing
-------
unit tests passed.
Thanks,
Piyush Sharma
Re: Review Request 55110: DATAFU-106 Test files are currently created
in the
subdirectory folder (e.g. datafu-pig/input*). For better organization,
they should be created in a subdirectory.
Posted by Eyal Allweil <ey...@yahoo.com>.
> On Jan. 2, 2017, 11:29 a.m., Eyal Allweil wrote:
> > Hi Piyush,
> >
> > Thank you for your patch! It looks to me that it works fine - I ran our tests on Ubuntu, both from Eclipse and from the command line.
> >
> > I have two comments, one "real" and one just a typo:
> >
> > 1) From [this question](http://stackoverflow.com/a/840229/150992), it appears that changing the *user.dir* property in this way can be unpredictable - that it won't affect FileOutputStreams, for example. So although this works for our current tests (the FileOutputStream used in datafu.pig.linkanalysis.PageRank uses *File.createTempFile* instead of the working dir, so it's fine) I worry that this might not work in the future. Maybe add a comment about this in the *beforeClass* method?
> >
> > 2) "Location" is spelled as "loaction" in PigTests.java
> >
> > Cheers,
> > Eyal
>
> Piyush Sharma wrote:
> Hi Eyal,
>
> Thank you for reviewing it.
>
> 1) I've actually tried doing it without changing user.dir. The pigtests don't actually work that way (for eg: LOAD 'input'... searches for the file in the working directory and it apparently deciphers it to be the same as that of the jvm). Hence, this was the only possible way to solve the issue that I could think of.
>
> I also went through that stackoverflow question whilst creating the patch to resolve my suspicions about the same. Actually, as even the accepted answer says it does affect the subsequent file creations, which happens via a call to writeLinesToFile defined in the PigTests.java which always creates a new file, deleting the existing one if necessary, and then writes to it. Now since writeLinesToFile also implicitly uses FileOutputStream (via a FileWriter object) to write to files just created in the new user.dir, it seemed like changing the working directory did actually affect the FileOutPutStreams, atleast as far as our project goes it did. Moreover the afterMethod resets the user.dir so hopefully there aren't any consequences at all.
>
> I can add some comment like "//Only some classes reflect the changes in working directory" or something like "//Developers are requested to just work with absoluteFiles and absolutePaths to avoid IOExceptions and such" but I feel it throws up a red flag unnecessarily, so should I ?
>
> 2) I will surely correct the typo, thank you.
>
> Cheerio,
> Piyush
I think this is fine - after all, all existing tests pass. One more very minor comment - I think I'd just put the test in the main java/datafu/test dir - I don't think there's a need for a subdirectory for it.
- Eyal
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55110/#review160317
-----------------------------------------------------------
On Jan. 6, 2017, 10:51 p.m., Piyush Sharma wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55110/
> -----------------------------------------------------------
>
> (Updated Jan. 6, 2017, 10:51 p.m.)
>
>
> Review request for DataFu and Matthew Hayes.
>
>
> Repository: datafu
>
>
> Description
> -------
>
> DATAFU-106: Test files are currently created in the subdirectory folder (e.g. datafu-pig/input*). For better organization, they should be created in a subdirectory. This also makes it easier to exclude them all with gitignore.
> (issue: https://issues.apache.org/jira/browse/DATAFU-106)
>
>
> Diffs
> -----
>
> datafu-pig/src/test/java/datafu/test/pig/PigTests.java d1d6fcc
> datafu-pig/src/test/java/datafu/test/pig/test_filesSubdir/TestFilesSubdirTest.java PRE-CREATION
>
> Diff: https://reviews.apache.org/r/55110/diff/
>
>
> Testing
> -------
>
> unit tests passed.
>
>
> Thanks,
>
> Piyush Sharma
>
>
Re: Review Request 55110: DATAFU-106 Test files are currently created
in the
subdirectory folder (e.g. datafu-pig/input*). For better organization,
they should be created in a subdirectory.
Posted by Piyush Sharma <ps...@gmail.com>.
> On Jan. 2, 2017, 9:29 a.m., Eyal Allweil wrote:
> > Hi Piyush,
> >
> > Thank you for your patch! It looks to me that it works fine - I ran our tests on Ubuntu, both from Eclipse and from the command line.
> >
> > I have two comments, one "real" and one just a typo:
> >
> > 1) From [this question](http://stackoverflow.com/a/840229/150992), it appears that changing the *user.dir* property in this way can be unpredictable - that it won't affect FileOutputStreams, for example. So although this works for our current tests (the FileOutputStream used in datafu.pig.linkanalysis.PageRank uses *File.createTempFile* instead of the working dir, so it's fine) I worry that this might not work in the future. Maybe add a comment about this in the *beforeClass* method?
> >
> > 2) "Location" is spelled as "loaction" in PigTests.java
> >
> > Cheers,
> > Eyal
Hi Eyal,
Thank you for reviewing it.
1) I've actually tried doing it without changing user.dir. The pigtests don't actually work that way (for eg: LOAD 'input'... searches for the file in the working directory and it apparently deciphers it to be the same as that of the jvm). Hence, this was the only possible way to solve the issue that I could think of.
I also went through that stackoverflow question whilst creating the patch to resolve my suspicions about the same. Actually, as even the accepted answer says it does affect the subsequent file creations, which happens via a call to writeLinesToFile defined in the PigTests.java which always creates a new file, deleting the existing one if necessary, and then writes to it. Now since writeLinesToFile also implicitly uses FileOutputStream (via a FileWriter object) to write to files just created in the new user.dir, it seemed like changing the working directory did actually affect the FileOutPutStreams, atleast as far as our project goes it did. Moreover the afterMethod resets the user.dir so hopefully there aren't any consequences at all.
I can add some comment like "//Only some classes reflect the changes in working directory" or something like "//Developers are requested to just work with absoluteFiles and absolutePaths to avoid IOExceptions and such" but I feel it throws up a red flag unnecessarily, so should I ?
2) I will surely correct the typo, thank you.
Cheerio,
Piyush
- Piyush
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55110/#review160317
-----------------------------------------------------------
On Dec. 31, 2016, 10:47 a.m., Piyush Sharma wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55110/
> -----------------------------------------------------------
>
> (Updated Dec. 31, 2016, 10:47 a.m.)
>
>
> Review request for DataFu.
>
>
> Repository: datafu
>
>
> Description
> -------
>
> DATAFU-106: Test files are currently created in the subdirectory folder (e.g. datafu-pig/input*). For better organization, they should be created in a subdirectory. This also makes it easier to exclude them all with gitignore.
> (issue: https://issues.apache.org/jira/browse/DATAFU-106)
>
>
> Diffs
> -----
>
> datafu-pig/src/test/java/datafu/test/pig/PigTests.java d1d6fcc
> datafu-pig/src/test/java/datafu/test/pig/test_filesSubdir/TestFilesSubdirTest.java PRE-CREATION
>
> Diff: https://reviews.apache.org/r/55110/diff/
>
>
> Testing
> -------
>
> unit tests passed.
>
>
> Thanks,
>
> Piyush Sharma
>
>
Re: Review Request 55110: DATAFU-106 Test files are currently created
in the
subdirectory folder (e.g. datafu-pig/input*). For better organization,
they should be created in a subdirectory.
Posted by Piyush Sharma <ps...@gmail.com>.
> On Jan. 2, 2017, 9:29 a.m., Eyal Allweil wrote:
> > Hi Piyush,
> >
> > Thank you for your patch! It looks to me that it works fine - I ran our tests on Ubuntu, both from Eclipse and from the command line.
> >
> > I have two comments, one "real" and one just a typo:
> >
> > 1) From [this question](http://stackoverflow.com/a/840229/150992), it appears that changing the *user.dir* property in this way can be unpredictable - that it won't affect FileOutputStreams, for example. So although this works for our current tests (the FileOutputStream used in datafu.pig.linkanalysis.PageRank uses *File.createTempFile* instead of the working dir, so it's fine) I worry that this might not work in the future. Maybe add a comment about this in the *beforeClass* method?
> >
> > 2) "Location" is spelled as "loaction" in PigTests.java
> >
> > Cheers,
> > Eyal
>
> Piyush Sharma wrote:
> Hi Eyal,
>
> Thank you for reviewing it.
>
> 1) I've actually tried doing it without changing user.dir. The pigtests don't actually work that way (for eg: LOAD 'input'... searches for the file in the working directory and it apparently deciphers it to be the same as that of the jvm). Hence, this was the only possible way to solve the issue that I could think of.
>
> I also went through that stackoverflow question whilst creating the patch to resolve my suspicions about the same. Actually, as even the accepted answer says it does affect the subsequent file creations, which happens via a call to writeLinesToFile defined in the PigTests.java which always creates a new file, deleting the existing one if necessary, and then writes to it. Now since writeLinesToFile also implicitly uses FileOutputStream (via a FileWriter object) to write to files just created in the new user.dir, it seemed like changing the working directory did actually affect the FileOutPutStreams, atleast as far as our project goes it did. Moreover the afterMethod resets the user.dir so hopefully there aren't any consequences at all.
>
> I can add some comment like "//Only some classes reflect the changes in working directory" or something like "//Developers are requested to just work with absoluteFiles and absolutePaths to avoid IOExceptions and such" but I feel it throws up a red flag unnecessarily, so should I ?
>
> 2) I will surely correct the typo, thank you.
>
> Cheerio,
> Piyush
>
> Eyal Allweil wrote:
> I think this is fine - after all, all existing tests pass. One more very minor comment - I think I'd just put the test in the main java/datafu/test dir - I don't think there's a need for a subdirectory for it.
Yeah sure, I've transferred the unit test to the datafu-pig/src/test/java/datafu/test directory and also corrected the typo in PigTests.java in the new patch that I've uploaded.
Cheerio
- Piyush
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55110/#review160317
-----------------------------------------------------------
On Jan. 6, 2017, 8:51 p.m., Piyush Sharma wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55110/
> -----------------------------------------------------------
>
> (Updated Jan. 6, 2017, 8:51 p.m.)
>
>
> Review request for DataFu and Matthew Hayes.
>
>
> Repository: datafu
>
>
> Description
> -------
>
> DATAFU-106: Test files are currently created in the subdirectory folder (e.g. datafu-pig/input*). For better organization, they should be created in a subdirectory. This also makes it easier to exclude them all with gitignore.
> (issue: https://issues.apache.org/jira/browse/DATAFU-106)
>
>
> Diffs
> -----
>
> datafu-pig/src/test/java/datafu/test/pig/PigTests.java d1d6fcc
> datafu-pig/src/test/java/datafu/test/pig/test_filesSubdir/TestFilesSubdirTest.java PRE-CREATION
>
> Diff: https://reviews.apache.org/r/55110/diff/
>
>
> Testing
> -------
>
> unit tests passed.
>
>
> Thanks,
>
> Piyush Sharma
>
>
Re: Review Request 55110: DATAFU-106 Test files are currently created
in the
subdirectory folder (e.g. datafu-pig/input*). For better organization,
they should be created in a subdirectory.
Posted by Eyal Allweil <ey...@yahoo.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55110/#review160317
-----------------------------------------------------------
Hi Piyush,
Thank you for your patch! It looks to me that it works fine - I ran our tests on Ubuntu, both from Eclipse and from the command line.
I have two comments, one "real" and one just a typo:
1) From [this question](http://stackoverflow.com/a/840229/150992), it appears that changing the *user.dir* property in this way can be unpredictable - that it won't affect FileOutputStreams, for example. So although this works for our current tests (the FileOutputStream used in datafu.pig.linkanalysis.PageRank uses *File.createTempFile* instead of the working dir, so it's fine) I worry that this might not work in the future. Maybe add a comment about this in the *beforeClass* method?
2) "Location" is spelled as "loaction" in PigTests.java
Cheers,
Eyal
- Eyal Allweil
On Dec. 31, 2016, 12:47 p.m., Piyush Sharma wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55110/
> -----------------------------------------------------------
>
> (Updated Dec. 31, 2016, 12:47 p.m.)
>
>
> Review request for DataFu.
>
>
> Repository: datafu
>
>
> Description
> -------
>
> DATAFU-106: Test files are currently created in the subdirectory folder (e.g. datafu-pig/input*). For better organization, they should be created in a subdirectory. This also makes it easier to exclude them all with gitignore.
> (issue: https://issues.apache.org/jira/browse/DATAFU-106)
>
>
> Diffs
> -----
>
> datafu-pig/src/test/java/datafu/test/pig/PigTests.java d1d6fcc
> datafu-pig/src/test/java/datafu/test/pig/test_filesSubdir/TestFilesSubdirTest.java PRE-CREATION
>
> Diff: https://reviews.apache.org/r/55110/diff/
>
>
> Testing
> -------
>
> unit tests passed.
>
>
> Thanks,
>
> Piyush Sharma
>
>