Home | About | Sematext search-lucene.com search-hadoop.com
 Search Hadoop and all its subprojects:

Switch to Threaded View
Sqoop >> mail # dev >> Review Request: Fix Sqoop unit test failures on Windows


Copy link to this message
-
Re: Review Request: Fix Sqoop unit test failures on Windows


> On April 5, 2013, 4:21 p.m., Venkat Ranganathan wrote:
> > src/java/org/apache/sqoop/util/ClassLoaderStack.java, line 78
> > <https://reviews.apache.org/r/10229/diff/1/?file=276974#file276974line78>
> >
> >     Why is the JarURL format getting changed.   File(jarFile).toURI().toURL() will still have the scheme as file right?

The objective is to construct the URL path using URL class which resolves the failures on Windows due to the preceding "jar:" and ending "!/" parts. I believe this is also favorable than handcrafting the URL string.

Verified this is working on both Windows and Linux
> On April 5, 2013, 4:21 p.m., Venkat Ranganathan wrote:
> > src/test/com/cloudera/sqoop/hive/TestHiveImport.java, line 192
> > <https://reviews.apache.org/r/10229/diff/1/?file=276975#file276975line192>
> >
> >     Nit: Trailing spaces

Did not see a trailing space in this line, but fixed a trailing space in the next one.
Thanks
> On April 5, 2013, 4:21 p.m., Venkat Ranganathan wrote:
> > src/test/com/cloudera/sqoop/lib/TestClobRef.java, line 105
> > <https://reviews.apache.org/r/10229/diff/1/?file=276978#file276978line105>
> >
> >     I am not sure the recursive delete can affect other test behavior.  I suggest not change additional generic behavior as part of this patch and address them in separate JIRAs
> >    
> >     Another question.
> >           String tmpDir = System.getProperty("test.build.data", "/tmp/");
> >    
> >     Also, did you want to change the above for windows as /tmp may not be always available in the current drive).  I was wondering something like getting the environment variable TMPDIR and use it instead of /tmp.
> >

Thank you Venkat,
I am a bit unclear here what difference is recursive delete making to the test if it just ensure the folder is cleaned even if it is not empty. I have verified all tests are passing both on Windows and Linux with this change. I am open to skipping this one into a different Jira, but I am not sure how is this needed given it is a small change in Test code, and affecting pass rate.

As for the /tmp/ thing, yes I agree with you, but I did not want to make a new change since that test.build.data is always set when this test runs. Do you mind tracking this separately since this change focuses on fixing unit test failures on Windows

Thanks,
Ahmed
- Ahmed
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/10229/#review18705
-----------------------------------------------------------
On April 1, 2013, 8:48 p.m., Ahmed El Baz wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/10229/
> -----------------------------------------------------------
>
> (Updated April 1, 2013, 8:48 p.m.)
>
>
> Review request for Sqoop.
>
>
> Description
> -------
>
> Please see SQOOP-955 for details
>
>
> This addresses bug SQOOP-955.
>     https://issues.apache.org/jira/browse/SQOOP-955
>
>
> Diffs
> -----
>
>   src/java/org/apache/sqoop/hive/HiveImport.java ce34286
>   src/java/org/apache/sqoop/orm/CompilationManager.java 92effb5
>   src/java/org/apache/sqoop/util/ClassLoaderStack.java 8e41cb3
>   src/test/com/cloudera/sqoop/hive/TestHiveImport.java 170bc66
>   src/test/com/cloudera/sqoop/io/TestNamedFifo.java 9f0a585
>   src/test/com/cloudera/sqoop/lib/TestBlobRef.java 2054a9b
>   src/test/com/cloudera/sqoop/lib/TestClobRef.java 8cb05e9
>   src/test/com/cloudera/sqoop/orm/TestClassWriter.java 3b77571
>   testdata/hive/bin/hive.cmd PRE-CREATION
>
> Diff: https://reviews.apache.org/r/10229/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Ahmed El Baz
>
>