-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/65530/#review197523
-----------------------------------------------------------


Ship it!




Hi Dani,

The fix works for me. 
Now, all we need is a committer to agree as well. ;)

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

Reply via email to