> 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 > > Fero Szabo wrote: > I have to add that I tested your diff through DirectMySQLExportTest, so > it might make sense to return the output of whoami elsewhere...
Hi Fero! Thanks for your review! I've updated the patch to fallback to checking the user.name system property instead of calling whoami and to throw an exception if none of them works. This helps in my original docker setup as user.name is set. The getCurrentUser() method was only called from MySQLTestUtils class so I've made it private. Let me know what you think! Regards, Daniel P.S. I think the Process documentation is talking about the other way around; when you explicitly set a Process variable to null, it won't be destroyed. I believe setting it to null is not necessary for the subprocess to terminate, calling waitFor() will block until it exits. - daniel ----------------------------------------------------------- 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/2/ > > > 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 > >
