-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/65530/#review197565
-----------------------------------------------------------
Ship it!
Nice and clean solution, and a very nice catch in the test codebase!
GJ!
- Attila 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
>
>
|