----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/43732/#review120357 -----------------------------------------------------------
Thanks for the contribution! I added a few comments below. Thanks gradle.properties (line 18) <https://reviews.apache.org/r/43732/#comment181788> Please keep the -SNAPSHOT suffix here. gradle/dependency-versions.gradle (line 23) <https://reviews.apache.org/r/43732/#comment181789> There is SAMZA-878 opened for this jackson library upgrade. You can associate the patch w/ that ticket as well. gradle/dependency-versions.gradle (line 32) <https://reviews.apache.org/r/43732/#comment181790> Please remove -MINE. gradle/dependency-versions.gradle (line 33) <https://reviews.apache.org/r/43732/#comment181791> The minimum supported YARN version is still 2.6.1 in Samza and we have not deprecated the support yet. Hence, I am curious why you need 2.7.1 here? samza-hdfs/src/main/scala/org/apache/samza/system/hdfs/HdfsConfig.scala (line 50) <https://reviews.apache.org/r/43732/#comment181797> Adding configuration variables also requires adding documentation to docs/learn/documentation/versioned/jobs/configuration-table.html. samza-hdfs/src/main/scala/org/apache/samza/system/hdfs/writer/AvroDataFileHdfsWriter.scala (line 30) <https://reviews.apache.org/r/43732/#comment181792> Please remove the auto-generated author info. Apache does not allow this. samza-hdfs/src/main/scala/org/apache/samza/system/hdfs/writer/AvroDataFileHdfsWriter.scala (line 33) <https://reviews.apache.org/r/43732/#comment181796> How is this class used? It would be nice to create some javadoc here to explain it. A set of unit tests for the new class is also needed here. samza-kv-rocksdb/src/main/java/org/apache/samza/storage/kv/RocksDbOptionsHelper.java (line 19) <https://reviews.apache.org/r/43732/#comment181795> This does not seem to be relevant to the AvroDataFileWriter? Can we move it to another patch? - Yi Pan (Data Infrastructure) On Feb. 18, 2016, 7:46 p.m., Edi Bice wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/43732/ > ----------------------------------------------------------- > > (Updated Feb. 18, 2016, 7:46 p.m.) > > > Review request for samza. > > > Repository: samza > > > Description > ------- > > https://issues.apache.org/jira/browse/SAMZA-876 > > Implemented AvroDataFileHdfsWriter fashioned loosely after > BinarySequenceFileHDFSWriter. > > Exposed several RocksDb configuration options (recommended in RocksDb tuning > guide): > > rocksdb.log.level > rocksdb.log.keepfilenum > rocksdb.log.timetoroll > rocksdb.log.maxfilesize > > rocksdb.bloomfilter.bits > rocksdb.max.background.compactions > rocksdb.max.background.flushes > rocksdb.num.write.buffers > rocksdb.target.file.size.base > rocksdb.max.bytes.level.base > > > Diffs > ----- > > gradle.properties 16e1f5d > gradle/dependency-versions.gradle 52e25aa > samza-hdfs/src/main/scala/org/apache/samza/system/hdfs/HdfsConfig.scala > 7993119 > > samza-hdfs/src/main/scala/org/apache/samza/system/hdfs/writer/AvroDataFileHdfsWriter.scala > PRE-CREATION > > samza-kv-rocksdb/src/main/java/org/apache/samza/storage/kv/RocksDbOptionsHelper.java > d4f765c > > Diff: https://reviews.apache.org/r/43732/diff/ > > > Testing > ------- > > I am using AvroDataFileHdfsWriter at the end of my pipeline. I feed the > generated avro files to Apache Samoa. Have processed millions of records > successfully. The RocksDb config changes are older and were used and verified > to be working when originally implemented. > > > Thanks, > > Edi Bice > >
