> 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 > testdata/hive/bin/hive.cmd PRE-CREATION > > Diff: https://reviews.apache.org/r/10229/diff/ > > > Testing > ------- > > > Thanks, > > Ahmed El Baz > >
