sqoop-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From daniel voros <daniel.vo...@gmail.com>
Subject Re: Review Request 65530: MySQL thirdparty tests hang if there's no USER environment variable
Date Tue, 13 Feb 2018 14:48:35 GMT


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


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