Re: Review Request 51142: SAMZA-967: HDFS System Consumer

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/51142/ --- (Updated Oct. 5, 2016, 6:16 p.m.) Review request for samza, Yi Pan (Data

Re: Review Request 51142: SAMZA-967: HDFS System Consumer

> On Sept. 29, 2016, 10:02 p.m., Yi Pan (Data Infrastructure) wrote: > > samza-hdfs/src/main/scala/org/apache/samza/system/hdfs/HdfsConfig.scala, > > line 197 > > > > > > Thinking of this more, I would prefer

Re: Review Request 51142: SAMZA-967: HDFS System Consumer

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/51142/ --- (Updated Oct. 5, 2016, 12:02 a.m.) Review request for samza, Yi Pan (Data

Re: Review Request 51142: SAMZA-967: HDFS System Consumer

> On Sept. 29, 2016, 10:02 p.m., Yi Pan (Data Infrastructure) wrote: > > samza-hdfs/src/main/scala/org/apache/samza/system/hdfs/HdfsConfig.scala, > > line 197 > > > > > > Thinking of this more, I would prefer

Re: Review Request 51142: SAMZA-967: HDFS System Consumer

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/51142/ --- (Updated Oct. 3, 2016, 4:54 p.m.) Review request for samza, Yi Pan (Data

Re: Review Request 51142: SAMZA-967: HDFS System Consumer

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/51142/ --- (Updated Oct. 3, 2016, 4:04 p.m.) Review request for samza, Yi Pan (Data

Re: Review Request 51142: SAMZA-967: HDFS System Consumer

> On Sept. 29, 2016, 10:06 p.m., Yi Pan (Data Infrastructure) wrote: > > samza-shell/src/main/bash/run-job-for-azkaban.sh, line 1 > > > > > > Question: why do we need this in open source? Don't we already have a > >

Re: Review Request 51142: SAMZA-967: HDFS System Consumer

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/51142/#review150953 --- samza-shell/src/main/bash/run-job-for-azkaban.sh (line 1)

Re: Review Request 51142: SAMZA-967: HDFS System Consumer

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/51142/#review150949 --- Fix it, then Ship it! Overall looks pretty good to me. Just a

Re: Review Request 51142: SAMZA-967: HDFS System Consumer

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/51142/#review150895 --- Still in the middle (MultiFileHdfsReader). Will continue after

Re: Review Request 51142: SAMZA-967: HDFS System Consumer

> On Sept. 14, 2016, 6:19 a.m., Yi Pan (Data Infrastructure) wrote: > > samza-hdfs/src/main/scala/org/apache/samza/system/hdfs/HdfsSystemFactory.scala, > > line 38 > > > > > > Not related to your RB, but could you

Re: Review Request 51142: SAMZA-967: HDFS System Consumer

> On Sept. 14, 2016, 6:19 a.m., Yi Pan (Data Infrastructure) wrote: > > samza-hdfs/src/main/scala/org/apache/samza/system/hdfs/HdfsSystemFactory.scala, > > line 38 > > > > > > Not related to your RB, but could you

Re: Review Request 51142: SAMZA-967: HDFS System Consumer

> On Sept. 29, 2016, 5:56 p.m., Prateek Maheshwari wrote: > > samza-hdfs/src/main/scala/org/apache/samza/system/hdfs/HdfsConfig.scala, > > line 66 > > > > > > "systems.%s.consumer.buffer-capacity" makes sense to me.

Re: Review Request 51142: SAMZA-967: HDFS System Consumer

> On Sept. 29, 2016, 5:56 p.m., Prateek Maheshwari wrote: > > samza-hdfs/src/main/scala/org/apache/samza/system/hdfs/HdfsConfig.scala, > > line 66 > > > > > > "systems.%s.consumer.buffer-capacity" makes sense to me.

Re: Review Request 51142: SAMZA-967: HDFS System Consumer

> On Sept. 29, 2016, 10:56 a.m., Prateek Maheshwari wrote: > > samza-hdfs/src/main/scala/org/apache/samza/system/hdfs/HdfsConfig.scala, > > line 66 > > > > > > "systems.%s.consumer.buffer-capacity" makes sense to

Re: Review Request 51142: SAMZA-967: HDFS System Consumer

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

Re: Review Request 51142: SAMZA-967: HDFS System Consumer

> On Sept. 28, 2016, 12:28 a.m., Navina Ramesh wrote: > > build.gradle, line 308 > > > > > > why is this dependency needed here? It seems like this compile > > dependency is required for samza-hdfs and now

Re: Review Request 51142: SAMZA-967: HDFS System Consumer

> On Sept. 14, 2016, 6:19 a.m., Yi Pan (Data Infrastructure) wrote: > > samza-hdfs/src/main/scala/org/apache/samza/system/hdfs/HdfsConfig.scala, > > line 66 > > > > > > It would be nicer to make it conforming to

Re: Review Request 51142: SAMZA-967: HDFS System Consumer

> On Sept. 14, 2016, 6:19 a.m., Yi Pan (Data Infrastructure) wrote: > > samza-hdfs/src/main/java/org/apache/samza/system/hdfs/reader/MultiFileHdfsReader.java, > > line 59 > > > > > > Not sure what are we doing here?

Re: Review Request 51142: SAMZA-967: HDFS System Consumer

> On Sept. 13, 2016, 1:37 a.m., Yi Pan (Data Infrastructure) wrote: > > samza-hdfs/src/main/java/org/apache/samza/system/hdfs/partitioner/DirectoryPartitioner.java, > > line 58 > > > > > > nit: since the input

Re: Review Request 51142: SAMZA-967: HDFS System Consumer

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/51142/ --- (Updated Sept. 28, 2016, 7:45 p.m.) Review request for samza, Chris Pettitt,

Re: Review Request 51142: SAMZA-967: HDFS System Consumer

> On Sept. 28, 2016, 12:28 a.m., Navina Ramesh wrote: > > build.gradle, line 308 > > > > > > why is this dependency needed here? It seems like this compile > > dependency is required for samza-hdfs and now

Re: Review Request 51142: SAMZA-967: HDFS System Consumer

> On Sept. 27, 2016, 11:11 p.m., Navina Ramesh wrote: > > samza-hdfs/src/main/scala/org/apache/samza/system/hdfs/HdfsConfig.scala, > > line 70 > > > > > > what is the "default-partitioner"? Is it possible to have

Re: Review Request 51142: SAMZA-967: HDFS System Consumer

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/51142/#review150648 --- Fix it, then Ship it! lgtm +1 .. I think you were planning to

Re: Review Request 51142: SAMZA-967: HDFS System Consumer

> On Sept. 13, 2016, 1:37 a.m., Yi Pan (Data Infrastructure) wrote: > > samza-hdfs/src/main/java/org/apache/samza/system/hdfs/reader/AvroFileHdfsReader.java, > > line 24 > > > > > > One concern I had w/ this

Re: Review Request 51142: SAMZA-967: HDFS System Consumer

> On Sept. 13, 2016, 12:33 a.m., Yi Pan (Data Infrastructure) wrote: > > samza-hdfs/src/main/java/org/apache/samza/system/hdfs/HdfsSystemConsumer.java, > > line 101 > > > > > > Better, to avoid the wasteful remote

Re: Review Request 51142: SAMZA-967: HDFS System Consumer

> On Sept. 14, 2016, 6:19 a.m., Yi Pan (Data Infrastructure) wrote: > > samza-hdfs/src/main/java/org/apache/samza/system/hdfs/reader/MultiFileHdfsReader.java, > > line 59 > > > > > > Not sure what are we doing here?

Re: Review Request 51142: SAMZA-967: HDFS System Consumer

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

Re: Review Request 51142: SAMZA-967: HDFS System Consumer

> On Sept. 13, 2016, 1:37 a.m., Yi Pan (Data Infrastructure) wrote: > > samza-hdfs/src/main/java/org/apache/samza/system/hdfs/reader/AvroFileHdfsReader.java, > > line 24 > > > > > > One concern I had w/ this

Re: Review Request 51142: SAMZA-967: HDFS System Consumer

> On Sept. 13, 2016, 12:33 a.m., Yi Pan (Data Infrastructure) wrote: > > samza-hdfs/src/main/java/org/apache/samza/system/hdfs/HdfsSystemConsumer.java, > > line 101 > > > > > > Better, to avoid the wasteful remote

Re: Review Request 51142: SAMZA-967: HDFS System Consumer

> On Sept. 13, 2016, 12:33 a.m., Yi Pan (Data Infrastructure) wrote: > > samza-hdfs/src/main/java/org/apache/samza/system/hdfs/HdfsSystemConsumer.java, > > line 101 > > > > > > Better, to avoid the wasteful remote

Re: Review Request 51142: SAMZA-967: HDFS System Consumer

> On Sept. 13, 2016, 1:37 a.m., Yi Pan (Data Infrastructure) wrote: > > samza-hdfs/src/main/java/org/apache/samza/system/hdfs/reader/AvroFileHdfsReader.java, > > line 24 > > > > > > One concern I had w/ this

Re: Review Request 51142: SAMZA-967: HDFS System Consumer

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/51142/ --- (Updated Sept. 20, 2016, 11:22 p.m.) Review request for samza, Chris Pettitt,

Re: Review Request 51142: SAMZA-967: HDFS System Consumer

> On Sept. 13, 2016, 1:37 a.m., Yi Pan (Data Infrastructure) wrote: > > samza-hdfs/src/main/java/org/apache/samza/system/hdfs/partitioner/DirectoryPartitioner.java, > > line 83 > > > > > > Question: this seems to be

Re: Review Request 51142: SAMZA-967: HDFS System Consumer

> On Sept. 14, 2016, 6:19 a.m., Yi Pan (Data Infrastructure) wrote: > > samza-hdfs/src/test/java/org/apache/samza/system/hdfs/partitioner/TestDirectoryPartitioner.java, > > line 183 > > > > > > It would be better

Re: Review Request 51142: SAMZA-967: HDFS System Consumer

> On Sept. 13, 2016, 12:33 a.m., Yi Pan (Data Infrastructure) wrote: > > samza-hdfs/src/main/java/org/apache/samza/system/hdfs/HdfsSystemConsumer.java, > > line 142 > > > > > > Isn't it clearer to have one loop

Re: Review Request 51142: SAMZA-967: HDFS System Consumer

> On Sept. 13, 2016, 1:37 a.m., Yi Pan (Data Infrastructure) wrote: > > samza-hdfs/src/main/java/org/apache/samza/system/hdfs/reader/AvroFileHdfsReader.java, > > line 24 > > > > > > One concern I had w/ this

Re: Review Request 51142: SAMZA-967: HDFS System Consumer

> On Sept. 13, 2016, 1:37 a.m., Yi Pan (Data Infrastructure) wrote: > > samza-hdfs/src/main/java/org/apache/samza/system/hdfs/partitioner/DirectoryPartitioner.java, > > line 83 > > > > > > Question: this seems to be

Re: Review Request 51142: SAMZA-967: HDFS System Consumer

> On Sept. 13, 2016, 12:33 a.m., Yi Pan (Data Infrastructure) wrote: > > samza-hdfs/src/main/java/org/apache/samza/system/hdfs/HdfsSystemAdmin.java, > > line 91 > > > > > > You can do: > > try(FSDataOutputStream

Re: Review Request 51142: SAMZA-967: HDFS System Consumer

> On Sept. 13, 2016, 1:37 a.m., Yi Pan (Data Infrastructure) wrote: > > samza-hdfs/src/main/java/org/apache/samza/system/hdfs/partitioner/DirectoryPartitioner.java, > > line 58 > > > > > > nit: since the input

Re: Review Request 51142: SAMZA-967: HDFS System Consumer

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/51142/#review148780 --- Thanks for pulling it off! Two high-level comments: a) I would

Re: Review Request 51142: SAMZA-967: HDFS System Consumer

> On Sept. 13, 2016, 12:33 a.m., Yi Pan (Data Infrastructure) wrote: > > Still in the middle but don't want to lose what I had up to now. Also in the middle of addressing all the feedbacks. Have all the changes locally. Will push them altogether later. Thanks again for your review! > On

Re: Review Request 51142: SAMZA-967: HDFS System Consumer

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/51142/#review148629 --- I am at HdfsReaderFactory. Will continue tomorrow.

Re: Review Request 51142: SAMZA-967: HDFS System Consumer

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/51142/#review148612 --- Still in the middle but don't want to lose what I had up to now.

Re: Review Request 51142: SAMZA-967: HDFS System Consumer

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/51142/ --- (Updated Sept. 9, 2016, 1:34 a.m.) Review request for samza, Chris Pettitt, Yi

Re: Review Request 51142: SAMZA-967: HDFS System Consumer

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/51142/ --- (Updated Sept. 9, 2016, 1:32 a.m.) Review request for samza, Chris Pettitt, Yi

Re: Review Request 51142: SAMZA-967: HDFS System Consumer

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/51142/ --- (Updated Sept. 9, 2016, 1:30 a.m.) Review request for samza, Chris Pettitt, Yi

Re: Review Request 51142: SAMZA-967: HDFS System Consumer

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/51142/ --- (Updated Sept. 7, 2016, 11:49 p.m.) Review request for samza, Chris Pettitt,

Re: Review Request 51142: SAMZA-967: HDFS System Consumer

> On Sept. 1, 2016, 9:21 p.m., Navina Ramesh wrote: > > @lhaiesp: Your patch looks awesome. Happy to review again once you have > > addressed the comments. It will be great if you can add some unit test for > > HdfsSystemConsumer. Some of the documentation that you will have to include > >

Re: Review Request 51142: SAMZA-967: HDFS System Consumer

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/51142/ --- (Updated Sept. 7, 2016, 11:44 p.m.) Review request for samza, Chris Pettitt,

Re: Review Request 51142: SAMZA-967: HDFS System Consumer

> On Aug. 31, 2016, 7:25 a.m., Navina Ramesh wrote: > > gradle/dependency-versions.gradle, line 39 > > > > > > Why is this dependency introduced? Is it possible to get rid of this > > dependency ? Yes. We can

Re: Review Request 51142: SAMZA-967: HDFS System Consumer

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/51142/#review147596 --- @lhaiesp: Your patch looks awesome. Happy to review again once

Re: Review Request 51142: SAMZA-967: HDFS System Consumer

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/51142/#review147414 --- Still reviewing. Will continue tomorrow. Thanks!

Re: Review Request 51142: SAMZA-967: HDFS System Consumer

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/51142/ --- (Updated Aug. 23, 2016, 11:20 p.m.) Review request for samza. Changes