----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/41702/#review125122 -----------------------------------------------------------
flume-ng-doc/sphinx/FlumeUserGuide.rst (line 1202) <https://reviews.apache.org/r/41702/#comment187859> Description needs to be updated accordingly. flume-ng-doc/sphinx/FlumeUserGuide.rst (line 1205) <https://reviews.apache.org/r/41702/#comment187860> Why don't we support regex subscription? flume-ng-doc/sphinx/FlumeUserGuide.rst (line 1223) <https://reviews.apache.org/r/41702/#comment187862> Can we add some more details here on how we handle failures, etc? flume-ng-doc/sphinx/FlumeUserGuide.rst (line 1247) <https://reviews.apache.org/r/41702/#comment187864> If we decide to add regex subscription support, we will have to add that as an example as well. flume-ng-sources/flume-kafka-source/src/main/java/org/apache/flume/source/kafka/KafkaSource.java (line 56) <https://reviews.apache.org/r/41702/#comment187865> May be mention that it is comma separated. Give an example. flume-ng-sources/flume-kafka-source/src/main/java/org/apache/flume/source/kafka/KafkaSource.java (line 85) <https://reviews.apache.org/r/41702/#comment187881> NIT: rename this to something like tpAndOffsetMetadata. flume-ng-sources/flume-kafka-source/src/main/java/org/apache/flume/source/kafka/KafkaSource.java (lines 111 - 113) <https://reviews.apache.org/r/41702/#comment187875> Really liked the way you have used nanoTime and currentTimeMillis, this is indeed the right way to use them AFAIK. flume-ng-sources/flume-kafka-source/src/main/java/org/apache/flume/source/kafka/KafkaSource.java (line 113) <https://reviews.apache.org/r/41702/#comment187872> NIT: may be rename batchEndTime to maxBatchEndTime flume-ng-sources/flume-kafka-source/src/main/java/org/apache/flume/source/kafka/KafkaSource.java (line 143) <https://reviews.apache.org/r/41702/#comment187873> it can still have no next, right? flume-ng-sources/flume-kafka-source/src/main/java/org/apache/flume/source/kafka/KafkaSource.java (lines 144 - 145) <https://reviews.apache.org/r/41702/#comment187874> NIT: inverse the order, seems counter-intuitive. flume-ng-sources/flume-kafka-source/src/main/java/org/apache/flume/source/kafka/KafkaSource.java (lines 157 - 158) <https://reviews.apache.org/r/41702/#comment187876> NIT: Can we combine these two lines? flume-ng-sources/flume-kafka-source/src/main/java/org/apache/flume/source/kafka/KafkaSource.java (line 185) <https://reviews.apache.org/r/41702/#comment187880> How is lock helping here? I think checking if toBeCommitted is empty or not before committing is a better way. flume-ng-sources/flume-kafka-source/src/main/java/org/apache/flume/source/kafka/KafkaSource.java (line 211) <https://reviews.apache.org/r/41702/#comment187882> this should be kafka.consumer, right? flume-ng-sources/flume-kafka-source/src/main/java/org/apache/flume/source/kafka/KafkaSource.java (line 213) <https://reviews.apache.org/r/41702/#comment187883> Did not make sense to me. Probably reword? flume-ng-sources/flume-kafka-source/src/main/java/org/apache/flume/source/kafka/KafkaSource.java (line 226) <https://reviews.apache.org/r/41702/#comment187884> File a JIRA, and add JIRA id here to track it. flume-ng-sources/flume-kafka-source/src/main/java/org/apache/flume/source/kafka/KafkaSource.java (lines 229 - 234) <https://reviews.apache.org/r/41702/#comment187885> Make sure there are unit tests for these cases. flume-ng-sources/flume-kafka-source/src/main/java/org/apache/flume/source/kafka/KafkaSource.java (line 283) <https://reviews.apache.org/r/41702/#comment187886> group id is marked as reqd. How can we get here? flume-ng-sources/flume-kafka-source/src/main/java/org/apache/flume/source/kafka/KafkaSource.java (line 314) <https://reviews.apache.org/r/41702/#comment187887> Does this catch even make sense now? I doubt. flume-ng-sources/flume-kafka-source/src/main/java/org/apache/flume/source/kafka/KafkaSource.java (line 333) <https://reviews.apache.org/r/41702/#comment187888> I think we can do without locks here. Please convince me otherwise. flume-ng-sources/flume-kafka-source/src/test/java/org/apache/flume/source/kafka/KafkaSourceEmbeddedKafka.java (line 77) <https://reviews.apache.org/r/41702/#comment187890> Avoid using hard coded localhost address. This can be obtained from inetaddress.getlocalhost(). flume-ng-sources/flume-kafka-source/src/test/java/org/apache/flume/source/kafka/KafkaSourceEmbeddedKafka.java (line 82) <https://reviews.apache.org/r/41702/#comment187891> Same as above, avoid using hard coded localhost. flume-ng-sources/flume-kafka-source/src/test/java/org/apache/flume/source/kafka/KafkaSourceEmbeddedKafka.java (line 114) <https://reviews.apache.org/r/41702/#comment187893> Same here. flume-ng-sources/flume-kafka-source/src/test/java/org/apache/flume/source/kafka/KafkaSourceEmbeddedZookeeper.java (line 53) <https://reviews.apache.org/r/41702/#comment187894> Same comment for address. flume-ng-sources/flume-kafka-source/src/test/java/org/apache/flume/source/kafka/TestKafkaSource.java (line 89) <https://reviews.apache.org/r/41702/#comment187895> Can we break this into multiple smaller tests? Would it make sense to do so? - Ashish Singh On March 23, 2016, 4:59 p.m., Grigoriy Rozhkov wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/41702/ > ----------------------------------------------------------- > > (Updated March 23, 2016, 4:59 p.m.) > > > Review request for Flume, Grant Henke, Jarek Cecho, and Jeff Holoman. > > > Repository: flume-git > > > Description > ------- > > Kafka Source support for kafka 0.9 > > > Diffs > ----- > > flume-ng-doc/sphinx/FlumeUserGuide.rst > 423e0cf8a3fffbdab6363f4f71df7fd8c3c133a3 > flume-ng-sources/flume-kafka-source/pom.xml > 0f93476c61e0281d45a15426493c4c3579503cee > > flume-ng-sources/flume-kafka-source/src/main/java/org/apache/flume/source/kafka/KafkaSource.java > fd1dd3c17bda875daaecef02cd7c537bb14242b8 > > flume-ng-sources/flume-kafka-source/src/main/java/org/apache/flume/source/kafka/KafkaSourceConstants.java > 911012cefcd656bac3308c3e02990a6ff42a0de5 > > flume-ng-sources/flume-kafka-source/src/main/java/org/apache/flume/source/kafka/KafkaSourceUtil.java > 4a4034bd826f67e790e16b67e5b52f469b182627 > > flume-ng-sources/flume-kafka-source/src/test/java/org/apache/flume/source/kafka/KafkaSourceEmbeddedKafka.java > 26c5c9d0aa29fbbc1d9eced70b26aa3f81855c26 > > flume-ng-sources/flume-kafka-source/src/test/java/org/apache/flume/source/kafka/KafkaSourceEmbeddedZookeeper.java > 1b8a27106e84b3ce1d8a100d31cf33d847c68f1b > > flume-ng-sources/flume-kafka-source/src/test/java/org/apache/flume/source/kafka/TestKafkaSource.java > 8ec14cccf555ab38449b1bcb1e41a6ecbd19fe7c > > flume-ng-sources/flume-kafka-source/src/test/java/org/apache/flume/source/kafka/TestKafkaSourceUtil.java > 0cbb4b69eca3b052a4b74a52a71ea3e45ce5f814 > pom.xml 15c086b5090305a1a70185626452a05cb7c1941d > > Diff: https://reviews.apache.org/r/41702/diff/ > > > Testing > ------- > > New features: > - multiple topic subscription > Style for configuring new Kafka Source is made following styles in these > patches. > https://reviews.apache.org/r/41626/ > https://reviews.apache.org/r/41629/ > > generate command: > git diff --no-prefix --full-index > FLUME-2821.patch > > apply command: > patch -p0 < FLUME-2821.patch > > > Thanks, > > Grigoriy Rozhkov > >
