----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/54528/#review205962 -----------------------------------------------------------
Hi Eric, Thanks for improving the patch, it looks good now! I have tested it manually and it worked fine but I suggest adding a unit test for DirCleanupHook, based on my comment below it should be straightforward to add it. Apart from that I think it would be great to clarify in the documentation which Sqoop tools are supported with this new option. You added the documentation to the import and mainframe-import tools which suggest that it works with these tools only but since the logic is added to the ClassWriter it should work with every tool which invokes the ClassWriter. Can you please clarify the supported tools in the documentation? Thanks, Szabolcs src/java/org/apache/sqoop/util/DirCleanupHook.java Lines 19 (patched) <https://reviews.apache.org/r/54528/#comment288886> Now that this logic is extracted it is easier to add a test case to it. I have done some experimenting with the TemporaryFolder JUnit rule and this works fine: package org.apache.sqoop.util; import org.junit.Before; import org.junit.Rule; import org.junit.Test; import org.junit.rules.TemporaryFolder; import static org.junit.Assert.assertFalse; public class TestDirCleanupHook { @Rule public TemporaryFolder tmpFolder = new TemporaryFolder(); private DirCleanupHook dirCleanupHook; @Before public void before() throws Exception { // Make sure the directory is not empty. tmpFolder.newFile(); dirCleanupHook = new DirCleanupHook(tmpFolder.getRoot().getAbsolutePath()); } @Test public void testDirCleanup() { dirCleanupHook.run(); assertFalse(tmpFolder.getRoot().exists()); } } Can you please add this or similar test case to your patch? - Szabolcs Vasas On July 11, 2018, 8:25 a.m., Eric Lin wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/54528/ > ----------------------------------------------------------- > > (Updated July 11, 2018, 8:25 a.m.) > > > Review request for Sqoop, Boglarka Egyed and Szabolcs Vasas. > > > Bugs: SQOOP-3042 > https://issues.apache.org/jira/browse/SQOOP-3042 > > > Repository: sqoop-trunk > > > Description > ------- > > After running sqoop, all the temp files generated by ClassWriter are left > behind on disk, so anyone can check those JAVA files to see the schema of > those tables that Sqoop has been interacting with. By default, the directory > is under /tmp/sqoop-<username>/compile. > > In class org.apache.sqoop.SqoopOptions, function getNonceJarDir(), I can see > that we did add "deleteOnExit" on the temp dir: > for (int attempts = 0; attempts < MAX_DIR_CREATE_ATTEMPTS; attempts++) { > hashDir = new File(baseDir, RandomHash.generateMD5String()); > while (hashDir.exists()) { > hashDir = new File(baseDir, RandomHash.generateMD5String()); > } > > if (hashDir.mkdirs()) { > // We created the directory. Use it. > // If this directory is not actually filled with files, delete it > // when the JVM quits. > hashDir.deleteOnExit(); > break; > } > } > However, I believe it failed to delete due to directory is not empty. > > > Diffs > ----- > > .gitignore 68cbe287 > src/docs/user/import-mainframe.txt abeb7cde > src/docs/user/import.txt 2d074f49 > src/java/org/apache/sqoop/SqoopOptions.java 3a19aeac > src/java/org/apache/sqoop/orm/ClassWriter.java a4a768af > src/java/org/apache/sqoop/orm/CompilationManager.java 6590cacc > src/java/org/apache/sqoop/tool/BaseSqoopTool.java 8d318327 > src/java/org/apache/sqoop/util/DirCleanupHook.java PRE-CREATION > > > Diff: https://reviews.apache.org/r/54528/diff/5/ > > > Testing > ------- > > I have tested manually. I have checked with a couple of other Java developers > and it turned out that it is not easy to add test for deleteOnExit, so I did > not add any test cases. The code path I changed does not seem to have test > coverage either. Let me know if I am wrong. > > Thanks > > > Thanks, > > Eric Lin > >
