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
Ahmed El Baz 2013-04-09, 00:33


> 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?
>
> Ahmed El Baz wrote:
>     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
>
> Venkat Ranganathan wrote:
>     Are you saying this does not work on Windows?
>    
>     String urlPath = "jar:file:" + new File(jarFile).getAbsolutePath() + "!/";

Correct. Using the crafted String urlPath is failing on Windows, thus the need to use the URL class.
> 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.
> >
>
> Ahmed El Baz wrote:
>     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
>
> Venkat Ranganathan wrote:
>     Sure.   I think it is good.  Generally though, it would be good to call it out explicitly in the file with a comment so that it does not get changed (by a merge) so developers know why it changed.   Adding a comment to that effect would be good

Sure thing. Done.
Please let me know if I need to upload the updated patch here or on the Jira.

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