----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/68316/#review207153 -----------------------------------------------------------
Hi Nguyen, Thank you for submitting this patch, this was a nasty little bug... Please see my comments below. src/java/org/apache/sqoop/manager/oracle/OraOopDBInputSplit.java Line 161 (original), 161 (patched) <https://reviews.apache.org/r/68316/#comment290446> It is true that your changes are covered with third party test cases but I think it would be great if you could add a few unit test cases to this method. In your tests you could initialize a OraOopDBInputSplit object with a few OraOopOracleDataChunk object and check if the getDebugDetails works as expected. src/java/org/apache/sqoop/manager/oracle/OraOopOracleDataChunkExtent.java Lines 24 (patched) <https://reviews.apache.org/r/68316/#comment290440> This seems to be an unnecessary whitespace change, please remove it. src/java/org/apache/sqoop/manager/oracle/OraOopOracleDataChunkExtent.java Lines 94 (patched) <https://reviews.apache.org/r/68316/#comment290443> I think we should log the id field as well which is inherited from the superclass. src/java/org/apache/sqoop/manager/oracle/OraOopOracleDataChunkExtent.java Lines 96 (patched) <https://reviews.apache.org/r/68316/#comment290441> nit: since you already have a StringBuilder object you could write: result.append("\n\t\t oracleDataObjectId = ").append(oracleDataObjectId); to avoid extra String object creation. src/java/org/apache/sqoop/manager/oracle/OraOopOracleDataChunkPartition.java Lines 78 (patched) <https://reviews.apache.org/r/68316/#comment290444> I think we should log the id field as well which is inherited from the superclass. - Szabolcs Vasas On Aug. 13, 2018, 9:14 a.m., Nguyen Truong wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/68316/ > ----------------------------------------------------------- > > (Updated Aug. 13, 2018, 9:14 a.m.) > > > Review request for Sqoop. > > > Bugs: SQOOP-3362 > https://issues.apache.org/jira/browse/SQOOP-3362 > > > Repository: sqoop-trunk > > > Description > ------- > > The method was currently returning the hash of data chunk object. I > implemented the toString() methods inside the subclasses of > OraOopOracleDataChunk. > > > Diffs > ----- > > src/java/org/apache/sqoop/manager/oracle/OraOopDBInputSplit.java 948bdbb73 > src/java/org/apache/sqoop/manager/oracle/OraOopOracleDataChunk.java > eb67fd2e4 > src/java/org/apache/sqoop/manager/oracle/OraOopOracleDataChunkExtent.java > 20b39eea0 > > src/java/org/apache/sqoop/manager/oracle/OraOopOracleDataChunkPartition.java > 59889b82b > > > Diff: https://reviews.apache.org/r/68316/diff/2/ > > > Testing > ------- > > No test case is added because the change has already covered. > > > Thanks, > > Nguyen Truong > >
