sqoop-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Jarek Cecho" <jar...@apache.org>
Subject Re: Review Request: SQOOP-807 Sqoop2: Verify whether job object can be safely removed prior removing
Date Wed, 09 Jan 2013 21:38:00 GMT


> On Jan. 9, 2013, 8:19 p.m., Cheolsoo Park wrote:
> > 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.

Hi Cheolsoo,
thank you very much for your review, I appreciate your time. I can't agree with your comment
more, so I've created SQOOP-823 to keep that note around.

Jarcec


- Jarek


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


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