> On Feb. 12, 2018, 4:50 p.m., Fero Szabo wrote: > > 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
I have to add that I tested your diff through DirectMySQLExportTest, so it might make sense to return the output of whoami elsewhere... - Fero ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/65530/#review197280 ----------------------------------------------------------- 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 > >
