----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/65530/#review197280 -----------------------------------------------------------
Hi Dani, Great catch! I like in your solution that it's greatly simplified compared to the original code. However, I believe that the process that executes the whoami command is never destroyed and hangs around in the background, according to the Process class' documentation (subprocesses with no reference to them are not destroyed): https://docs.oracle.com/javase/7/docs/api/java/lang/Process.html Probably this is why the original while loop existed (?) I'm really just guessing. Anyway, I find it strange to use whoami to get the username here, as this username is later on usedd by DirectMySQLManager. So this username is probably for the database, which is usually different than what whoami returns (at least on my system, it is). Better to throw an exception if it's not set? Cheers, Fero - Fero Szabo On Feb. 6, 2018, 12:15 p.m., daniel voros wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/65530/ > ----------------------------------------------------------- > > (Updated Feb. 6, 2018, 12:15 p.m.) > > > Review request for Sqoop. > > > Bugs: SQOOP-3283 > https://issues.apache.org/jira/browse/SQOOP-3283 > > > Repository: sqoop-trunk > > > Description > ------- > > `org.apache.sqoop.manager.mysql.MySQLTestUtils#getCurrentUser()` executes > `whoami` in a subprocess if there's no USER environment variable (happened to > me while running tests from Docker). However, it waits for the Process > variable to become null, that never happens: > > ``` > // wait for whoami to exit. > while (p != null) { > try { > int ret = p.waitFor(); > if (0 != ret) { > LOG.error("whoami exited with error status " + ret); > // suppress original return value from this method. > return null; > } > } catch (InterruptedException ie) { > continue; // loop around. > } > } > ``` > > We could get rid of the while loop since `Process#waitFor()` blocks while it > completes. > > Note, that it's easy to workaround the issue by setting the USER environment > variable when running the tests. > > > Diffs > ----- > > src/test/org/apache/sqoop/manager/mysql/MySQLTestUtils.java 25dbe9d > > > Diff: https://reviews.apache.org/r/65530/diff/1/ > > > Testing > ------- > > Run `org.apache.sqoop.manager.mysql.MySQLCompatTest`. Failed with timout > without the patch. All 46 test cases pass in ~45 seconds with the patch in > place. > > > Thanks, > > daniel voros > >
