sqoop-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Fero Szabo via Review Board <nore...@reviews.apache.org>
Subject Re: Review Request 65530: MySQL thirdparty tests hang if there's no USER environment variable
Date Mon, 12 Feb 2018 16:50:43 GMT

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



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


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


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message