You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@sqoop.apache.org by Ahmed El Baz <ah...@microsoft.com> on 2013/04/01 22:48:33 UTC
Review Request: Fix Sqoop unit test failures on Windows
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/10229/
-----------------------------------------------------------
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
Re: Review Request: Fix Sqoop unit test failures on Windows
Posted by Ahmed El Baz <ah...@microsoft.com>.
> 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() + "!/";
>
> Ahmed El Baz wrote:
> Correct. Using the crafted String urlPath is failing on Windows, thus the need to use the URL class.
>
> Venkat Ranganathan wrote:
> That is interesting. remember using it in my earlier work. I understand that you want to get it consistent across Unix and windows. Can you please see the content below and try it on Windows and Linux? I don't think the issue is because of jar: and !/ suffixes. The issue is because file urls crafted with file:// syntax expect the file part to start with / and not a drive letter. That is why using file: (without // after file) helps. In any case, URLClassLoader can work with Jar URLs without the prefix if we want the full jarfile contents. I think there is a windows issue with dynamically replacing jarfiles because of a Java bug. That is a different issue.
>
> ============
> import java.io.File;
> import java.net.JarURLConnection;
> import java.net.URL;
> import java.util.Map;
> import java.util.jar.Attributes;
> import java.util.jar.Manifest;
>
>
>
> public class JarURLConn {
>
> public static void main(String args[]) {
> if (args.length < 1) {
> System.out.println("Usage JavaURLConn jar1 ...");
> System.exit(1);
> }
> for (int i = 0; i < args.length; ++i) {
> System.out.println("Information on jar file " + args[i]);
> System.out.println("\n\n");
> try {
> printJarInfo(args[i]);
> }
> catch (Exception e) {
> System.out.println("Caught exception : " + e.toString());
> }
> }
> }
>
>
> private static void printJarInfo(String jarFile) throws Exception {
> File f = new File(jarFile);
> if (f.canRead()) {
> StringBuilder sb = new StringBuilder();
> sb.append("jar:file:");
> sb.append(f.getAbsolutePath());
> sb.append("!/");
> String jarURLStr = sb.toString();
> System.out.println("Jar URL is " + jarURLStr);
> URL url = new URL(jarURLStr);
> JarURLConnection conn = (JarURLConnection) url.openConnection();
> Manifest manifest = conn.getManifest();
> Map<String, Attributes> attrMap = manifest.getEntries();
>
> for (String key : attrMap.keySet()) {
> Attributes attrs = attrMap.get(key);
> for (Object attrKey : attrs.keySet()) {
> System.out.println("Attribute " + attrKey + " = "
> + attrs.get(attrKey));
> }
> }
>
> }
> else {
> System.out.println("Can't access jar file " + jarFile);
> }
>
> }
> }
> =======
>
> Ahmed El Baz wrote:
> Thanks Venkat,
> I have tried the sample above, and yes the error happens when we add the extra "://". Are you suggesting to change the fix to remove the slashes from the file:// part. I believe using URL APIs would abstract all those issues. Please let me know what you think
>
> Venkat Ranganathan wrote:
> I think to be safe (and correct in terms of usage and convention), please leave the scheme as jar - so the constructed URL (with your change for platform neutral way of creating a file URL) can be
> "jar:" + new File(jarFile).toURI().toURL().toString() + "!/"
Thank you Venkat,
I have did the change as discussed (by removing the file:// part) along with using the URL format of the path. The change works fine on Windiwos, and I have 100% pass rate.
My Linux box is down so I could not run the unit tests on it. Would you kindly be able to unpack the patch and run unit tests on Linux. I ran all tests on Linux as well and everything was passing, so I do not expect any failures.
Much appreciated,
Ahmed
- Ahmed
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/10229/#review18705
-----------------------------------------------------------
On April 12, 2013, 2:31 a.m., Ahmed El Baz wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/10229/
> -----------------------------------------------------------
>
> (Updated April 12, 2013, 2:31 a.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
>
>
Re: Review Request: Fix Sqoop unit test failures on Windows
Posted by Venkat Ranganathan <n....@live.com>.
> 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() + "!/";
>
> Ahmed El Baz wrote:
> Correct. Using the crafted String urlPath is failing on Windows, thus the need to use the URL class.
>
> Venkat Ranganathan wrote:
> That is interesting. remember using it in my earlier work. I understand that you want to get it consistent across Unix and windows. Can you please see the content below and try it on Windows and Linux? I don't think the issue is because of jar: and !/ suffixes. The issue is because file urls crafted with file:// syntax expect the file part to start with / and not a drive letter. That is why using file: (without // after file) helps. In any case, URLClassLoader can work with Jar URLs without the prefix if we want the full jarfile contents. I think there is a windows issue with dynamically replacing jarfiles because of a Java bug. That is a different issue.
>
> ============
> import java.io.File;
> import java.net.JarURLConnection;
> import java.net.URL;
> import java.util.Map;
> import java.util.jar.Attributes;
> import java.util.jar.Manifest;
>
>
>
> public class JarURLConn {
>
> public static void main(String args[]) {
> if (args.length < 1) {
> System.out.println("Usage JavaURLConn jar1 ...");
> System.exit(1);
> }
> for (int i = 0; i < args.length; ++i) {
> System.out.println("Information on jar file " + args[i]);
> System.out.println("\n\n");
> try {
> printJarInfo(args[i]);
> }
> catch (Exception e) {
> System.out.println("Caught exception : " + e.toString());
> }
> }
> }
>
>
> private static void printJarInfo(String jarFile) throws Exception {
> File f = new File(jarFile);
> if (f.canRead()) {
> StringBuilder sb = new StringBuilder();
> sb.append("jar:file:");
> sb.append(f.getAbsolutePath());
> sb.append("!/");
> String jarURLStr = sb.toString();
> System.out.println("Jar URL is " + jarURLStr);
> URL url = new URL(jarURLStr);
> JarURLConnection conn = (JarURLConnection) url.openConnection();
> Manifest manifest = conn.getManifest();
> Map<String, Attributes> attrMap = manifest.getEntries();
>
> for (String key : attrMap.keySet()) {
> Attributes attrs = attrMap.get(key);
> for (Object attrKey : attrs.keySet()) {
> System.out.println("Attribute " + attrKey + " = "
> + attrs.get(attrKey));
> }
> }
>
> }
> else {
> System.out.println("Can't access jar file " + jarFile);
> }
>
> }
> }
> =======
>
> Ahmed El Baz wrote:
> Thanks Venkat,
> I have tried the sample above, and yes the error happens when we add the extra "://". Are you suggesting to change the fix to remove the slashes from the file:// part. I believe using URL APIs would abstract all those issues. Please let me know what you think
I think to be safe (and correct in terms of usage and convention), please leave the scheme as jar - so the constructed URL (with your change for platform neutral way of creating a file URL) can be
"jar:" + new File(jarFile).toURI().toURL().toString() + "!/"
- Venkat
-----------------------------------------------------------
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
>
>
Re: Review Request: Fix Sqoop unit test failures on Windows
Posted by Ahmed El Baz <ah...@microsoft.com>.
> 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
>
>
Re: Review Request: Fix Sqoop unit test failures on Windows
Posted by Venkat Ranganathan <n....@live.com>.
> 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
Are you saying this does not work on Windows?
String urlPath = "jar:file:" + new File(jarFile).getAbsolutePath() + "!/";
> 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
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
- Venkat
-----------------------------------------------------------
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
>
>
Re: Review Request: Fix Sqoop unit test failures on Windows
Posted by Ahmed El Baz <ah...@microsoft.com>.
> 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() + "!/";
>
> Ahmed El Baz wrote:
> Correct. Using the crafted String urlPath is failing on Windows, thus the need to use the URL class.
>
> Venkat Ranganathan wrote:
> That is interesting. remember using it in my earlier work. I understand that you want to get it consistent across Unix and windows. Can you please see the content below and try it on Windows and Linux? I don't think the issue is because of jar: and !/ suffixes. The issue is because file urls crafted with file:// syntax expect the file part to start with / and not a drive letter. That is why using file: (without // after file) helps. In any case, URLClassLoader can work with Jar URLs without the prefix if we want the full jarfile contents. I think there is a windows issue with dynamically replacing jarfiles because of a Java bug. That is a different issue.
>
> ============
> import java.io.File;
> import java.net.JarURLConnection;
> import java.net.URL;
> import java.util.Map;
> import java.util.jar.Attributes;
> import java.util.jar.Manifest;
>
>
>
> public class JarURLConn {
>
> public static void main(String args[]) {
> if (args.length < 1) {
> System.out.println("Usage JavaURLConn jar1 ...");
> System.exit(1);
> }
> for (int i = 0; i < args.length; ++i) {
> System.out.println("Information on jar file " + args[i]);
> System.out.println("\n\n");
> try {
> printJarInfo(args[i]);
> }
> catch (Exception e) {
> System.out.println("Caught exception : " + e.toString());
> }
> }
> }
>
>
> private static void printJarInfo(String jarFile) throws Exception {
> File f = new File(jarFile);
> if (f.canRead()) {
> StringBuilder sb = new StringBuilder();
> sb.append("jar:file:");
> sb.append(f.getAbsolutePath());
> sb.append("!/");
> String jarURLStr = sb.toString();
> System.out.println("Jar URL is " + jarURLStr);
> URL url = new URL(jarURLStr);
> JarURLConnection conn = (JarURLConnection) url.openConnection();
> Manifest manifest = conn.getManifest();
> Map<String, Attributes> attrMap = manifest.getEntries();
>
> for (String key : attrMap.keySet()) {
> Attributes attrs = attrMap.get(key);
> for (Object attrKey : attrs.keySet()) {
> System.out.println("Attribute " + attrKey + " = "
> + attrs.get(attrKey));
> }
> }
>
> }
> else {
> System.out.println("Can't access jar file " + jarFile);
> }
>
> }
> }
> =======
Thanks Venkat,
I have tried the sample above, and yes the error happens when we add the extra "://". Are you suggesting to change the fix to remove the slashes from the file:// part. I believe using URL APIs would abstract all those issues. Please let me know what you think
> On April 5, 2013, 4:21 p.m., Venkat Ranganathan wrote:
> > src/test/com/cloudera/sqoop/orm/TestClassWriter.java, line 177
> > <https://reviews.apache.org/r/10229/diff/1/?file=276979#file276979line177>
> >
> > Nit - tabs instead of spaces
>
> Venkat Ranganathan wrote:
> Please check as mentioned above
Will do
Thanks
> 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
>
> Ahmed El Baz wrote:
> Did not see a trailing space in this line, but fixed a trailing space in the next one.
> Thanks
>
> Venkat Ranganathan wrote:
> Can you please make sure after a upload your diff to check if there are no red bars in the beginning or end. That will signify the space issues.
Will doThanks
- 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
>
>
Re: Review Request: Fix Sqoop unit test failures on Windows
Posted by Ahmed El Baz <ah...@microsoft.com>.
> 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
>
>
Re: Review Request: Fix Sqoop unit test failures on Windows
Posted by Venkat Ranganathan <n....@live.com>.
> 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() + "!/";
>
> Ahmed El Baz wrote:
> Correct. Using the crafted String urlPath is failing on Windows, thus the need to use the URL class.
That is interesting. remember using it in my earlier work. I understand that you want to get it consistent across Unix and windows. Can you please see the content below and try it on Windows and Linux? I don't think the issue is because of jar: and !/ suffixes. The issue is because file urls crafted with file:// syntax expect the file part to start with / and not a drive letter. That is why using file: (without // after file) helps. In any case, URLClassLoader can work with Jar URLs without the prefix if we want the full jarfile contents. I think there is a windows issue with dynamically replacing jarfiles because of a Java bug. That is a different issue.
============
import java.io.File;
import java.net.JarURLConnection;
import java.net.URL;
import java.util.Map;
import java.util.jar.Attributes;
import java.util.jar.Manifest;
public class JarURLConn {
public static void main(String args[]) {
if (args.length < 1) {
System.out.println("Usage JavaURLConn jar1 ...");
System.exit(1);
}
for (int i = 0; i < args.length; ++i) {
System.out.println("Information on jar file " + args[i]);
System.out.println("\n\n");
try {
printJarInfo(args[i]);
}
catch (Exception e) {
System.out.println("Caught exception : " + e.toString());
}
}
}
private static void printJarInfo(String jarFile) throws Exception {
File f = new File(jarFile);
if (f.canRead()) {
StringBuilder sb = new StringBuilder();
sb.append("jar:file:");
sb.append(f.getAbsolutePath());
sb.append("!/");
String jarURLStr = sb.toString();
System.out.println("Jar URL is " + jarURLStr);
URL url = new URL(jarURLStr);
JarURLConnection conn = (JarURLConnection) url.openConnection();
Manifest manifest = conn.getManifest();
Map<String, Attributes> attrMap = manifest.getEntries();
for (String key : attrMap.keySet()) {
Attributes attrs = attrMap.get(key);
for (Object attrKey : attrs.keySet()) {
System.out.println("Attribute " + attrKey + " = "
+ attrs.get(attrKey));
}
}
}
else {
System.out.println("Can't access jar file " + jarFile);
}
}
}
=======
> 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
>
> Ahmed El Baz wrote:
> Did not see a trailing space in this line, but fixed a trailing space in the next one.
> Thanks
Can you please make sure after a upload your diff to check if there are no red bars in the beginning or end. That will signify the space issues.
> On April 5, 2013, 4:21 p.m., Venkat Ranganathan wrote:
> > src/test/com/cloudera/sqoop/orm/TestClassWriter.java, line 177
> > <https://reviews.apache.org/r/10229/diff/1/?file=276979#file276979line177>
> >
> > Nit - tabs instead of spaces
Please check as mentioned above
- Venkat
-----------------------------------------------------------
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
>
>
Re: Review Request: Fix Sqoop unit test failures on Windows
Posted by Venkat Ranganathan <n....@live.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/10229/#review18705
-----------------------------------------------------------
Thanks for the changes. They look good. There are a few nits and couple of questions below. Thanks
src/java/org/apache/sqoop/util/ClassLoaderStack.java
<https://reviews.apache.org/r/10229/#comment39132>
Why is the JarURL format getting changed. File(jarFile).toURI().toURL() will still have the scheme as file right?
src/test/com/cloudera/sqoop/hive/TestHiveImport.java
<https://reviews.apache.org/r/10229/#comment39131>
Nit: Trailing spaces
src/test/com/cloudera/sqoop/io/TestNamedFifo.java
<https://reviews.apache.org/r/10229/#comment39133>
Good point on NamedFifo. Nit. Can you add log message saying that it is not supported
src/test/com/cloudera/sqoop/lib/TestClobRef.java
<https://reviews.apache.org/r/10229/#comment39135>
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.
src/test/com/cloudera/sqoop/orm/TestClassWriter.java
<https://reviews.apache.org/r/10229/#comment39134>
Nit - tabs instead of spaces
- Venkat Ranganathan
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
>
>
Re: Review Request: Fix Sqoop unit test failures on Windows
Posted by Jarek Cecho <ja...@apache.org>.
> On April 14, 2013, 4:57 a.m., Venkat Ranganathan wrote:
> > I have verified the changes. Thanks Ahmed for your contribution
> >
Thank you Ahmed for your contribution, please do upload final version of the patch to JIRA so that I can commit it.
- Jarek
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/10229/#review19133
-----------------------------------------------------------
On April 12, 2013, 2:31 a.m., Ahmed El Baz wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/10229/
> -----------------------------------------------------------
>
> (Updated April 12, 2013, 2:31 a.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
>
>
Re: Review Request: Fix Sqoop unit test failures on Windows
Posted by Ahmed El Baz <ah...@microsoft.com>.
> On April 14, 2013, 4:57 a.m., Venkat Ranganathan wrote:
> > I have verified the changes. Thanks Ahmed for your contribution
> >
>
> Jarek Cecho wrote:
> Thank you Ahmed for your contribution, please do upload final version of the patch to JIRA so that I can commit it.
Done.
Thanks everbody for your time and help revieing this patch
- Ahmed
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/10229/#review19133
-----------------------------------------------------------
On April 12, 2013, 2:31 a.m., Ahmed El Baz wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/10229/
> -----------------------------------------------------------
>
> (Updated April 12, 2013, 2:31 a.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
>
>
Re: Review Request: Fix Sqoop unit test failures on Windows
Posted by Venkat Ranganathan <n....@live.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/10229/#review19133
-----------------------------------------------------------
Ship it!
I have verified the changes. Thanks Ahmed for your contribution
- Venkat Ranganathan
On April 12, 2013, 2:31 a.m., Ahmed El Baz wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/10229/
> -----------------------------------------------------------
>
> (Updated April 12, 2013, 2:31 a.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
>
>
Re: Review Request: Fix Sqoop unit test failures on Windows
Posted by Ahmed El Baz <ah...@microsoft.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/10229/
-----------------------------------------------------------
(Updated April 12, 2013, 2:31 a.m.)
Review request for Sqoop.
Changes
-------
Addressing spacing and ClassLoader changes as discussed below.
Thanks,
Ahmed
Description
-------
Please see SQOOP-955 for details
This addresses bug SQOOP-955.
https://issues.apache.org/jira/browse/SQOOP-955
Diffs (updated)
-----
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