> On Dec. 22, 2014, 11: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 > > Qian Xu wrote: > 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`.
sure, for my education. what does this VisibleForTesting annotation buy us ? chaning to package private already allows me to use them in tests. is it just that the comment can be taken off? if so, happy to make that change in another RB - Veena ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/29319/#review65879 ----------------------------------------------------------- On Dec. 22, 2014, 2:33 p.m., Veena Basavaraj wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/29319/ > ----------------------------------------------------------- > > (Updated Dec. 22, 2014, 2:33 p.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 > >
