> On Dec. 23, 2014, 3:16 p.m., Qian Xu wrote: > > execution/mapreduce/src/main/java/org/apache/sqoop/job/io/SqoopWritable.java, > > line 52 > > <https://reviews.apache.org/r/29319/diff/3/?file=798942#file798942line52> > > > > 1. @VisibleForTesting is equivalent to the comment. I use it > > everywhere. Do you mind to use it to keep them consistent? > > 2. If the class is immutable, I'd suggest to create a testing visible > > consctructor `SqoopWritable(IntermediateDataFormat<?> dataFormat, String > > data)`. > > Abraham Elmahrek wrote: > Create follow ups for the VisibleForTesting annotation? > > Veena Basavaraj wrote: > @VisibleForTesting > protected KiteDatasetExecutor(Dataset<GenericRecord> dataset) { > this.dataset = dataset; > } > > why does it have to be protected for testing/ and it is a import > com.google.common.annotations.VisibleForTesting thing, and it is only used so > far in kite connector code, are we going to make this a standard? I would > like to understand what is the benefit of this vs simply making it package > default that so far is a common standard used across in many projects for > exposing methods just for testing > > Please send an email if we want to make this a standard and addit to > the coding guidelines wiki
As you and Ryan have commented in https://reviews.apache.org/r/29090/, I've changed related code to @VisibleForTesting KiteDatasetExecutor(Dataset<GenericRecord> dataset) { this.dataset = dataset; } Remove `protected` is expected. I'm suggesting adding @VisibleForTesting annotation and removing the comment `// default/package visibility for testing`. - Qian ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/29319/#review65879 ----------------------------------------------------------- On Dec. 23, 2014, 6:33 a.m., Veena Basavaraj wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/29319/ > ----------------------------------------------------------- > > (Updated Dec. 23, 2014, 6:33 a.m.) > > > Review request for Sqoop. > > > Bugs: SQOOP-1935 > https://issues.apache.org/jira/browse/SQOOP-1935 > > > Repository: sqoop-sqoop2 > > > Description > ------- > > see jira > > > Diffs > ----- > > > execution/mapreduce/src/main/java/org/apache/sqoop/job/io/SqoopWritable.java > 6a0bfa4 > > execution/mapreduce/src/main/java/org/apache/sqoop/job/mr/SqoopOutputFormatLoadExecutor.java > aaf771c > > execution/mapreduce/src/test/java/org/apache/sqoop/job/io/TestSqoopWritable.java > b07a076 > > execution/mapreduce/src/test/java/org/apache/sqoop/job/util/MRJobTestUtil.java > 5d5359e > > Diff: https://reviews.apache.org/r/29319/diff/ > > > Testing > ------- > > > Thanks, > > Veena Basavaraj > >
