sqoop-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Cheolsoo Park" <cheol...@cloudera.com>
Subject Re: Review Request: SQOOP-807 Sqoop2: Verify whether job object can be safely removed prior removing
Date Wed, 09 Jan 2013 20:19:07 GMT

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

Ship it!


The changes look good to me.

One minor comment that I had is that it would be helpful for a reviewer if you added some
comments why the these assertions hold. I was able to understand the reason only after reading
the loadSubmissions() method in DerbyTestCase.

assertTrue(handler.inUseJob(1, getDerbyConnection()));
assertFalse(handler.inUseJob(2, getDerbyConnection()));
assertFalse(handler.inUseJob(3, getDerbyConnection()));
assertFalse(handler.inUseJob(4, getDerbyConnection()));

Adding a simple comment like "// See DerbyTestCase regarding why these assertions must hold"
will be very helpful.

I don't see many comments in test cases now. I guess we can improve it in the future.

- Cheolsoo Park


On Jan. 2, 2013, 6:57 p.m., Jarek Cecho wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/8803/
> -----------------------------------------------------------
> 
> (Updated Jan. 2, 2013, 6:57 p.m.)
> 
> 
> Review request for Sqoop.
> 
> 
> Description
> -------
> 
> I've added proposed functionality.
> 
> 
> This addresses bug SQOOP-807.
>     https://issues.apache.org/jira/browse/SQOOP-807
> 
> 
> Diffs
> -----
> 
>   repository/repository-derby/src/main/java/org/apache/sqoop/repository/derby/DerbyRepositoryHandler.java
c62730ccd077663c763ece7df0c044acd437ffcc 
>   repository/repository-derby/src/test/java/org/apache/sqoop/repository/derby/TestConnectionHandling.java
4121be74364de33e0cae0aa1fb469a8c7d632922 
>   repository/repository-derby/src/test/java/org/apache/sqoop/repository/derby/TestJobHandling.java
c0e7a313e887e1fbca244ca92f7cf8b8a1d44bd4 
> 
> Diff: https://reviews.apache.org/r/8803/diff/
> 
> 
> Testing
> -------
> 
> I've added unit test + one unit test for similar function with Connection object.
> 
> 
> Thanks,
> 
> Jarek Cecho
> 
>


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