> 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
> 
>

Reply via email to