----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/51541/#review147430 -----------------------------------------------------------
flume-ng-sources/flume-kafka-source/src/main/java/org/apache/flume/source/kafka/KafkaSource.java (lines 126 - 127) <https://reviews.apache.org/r/51541/#comment214596> nit: no need to initialize with `null` flume-ng-sources/flume-kafka-source/src/main/java/org/apache/flume/source/kafka/KafkaSource.java (line 372) <https://reviews.apache.org/r/51541/#comment214597> What about using google guava's `Strings.isNullOrEmpty()` or `isEmpty()` in apache commons `StringUtils`? It's more readable. There are a couple of null-isEmpty checks, it might be worth to replace all of them. Or you might want to consider using the `context.getString`'s default value parameter. flume-ng-sources/flume-kafka-source/src/main/java/org/apache/flume/source/kafka/KafkaSource.java (line 394) <https://reviews.apache.org/r/51541/#comment214598> If Flume reconfigure occurs and the updated config file doesn't contain the `group.id` (`GROUP_ID_CONFIG`) entry it should use the default value, shouldn't it? I'm not sure though, as per the https://issues.apache.org/jira/browse/FLUME-2857 ticket this wouldn't be the first place where we introduce this. Wdyt? flume-ng-sources/flume-kafka-source/src/main/java/org/apache/flume/source/kafka/KafkaSource.java (line 433) <https://reviews.apache.org/r/51541/#comment214602> Could you please remove the `bootStrapServers` parameter? It's not needed as it was moved to instance level. flume-ng-sources/flume-kafka-source/src/main/java/org/apache/flume/source/kafka/KafkaSource.java (lines 530 - 598) <https://reviews.apache.org/r/51541/#comment214603> I'm a bit concerned about this copy-paste from the KafkaChannel it should be extracted to a utility class. If you don't have time for this now please file a jira to improve/refactor this later. Thanks. checkstyle & tests pass - Denes Arvay On Aug. 30, 2016, 9:47 p.m., Grant Henke wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/51541/ > ----------------------------------------------------------- > > (Updated Aug. 30, 2016, 9:47 p.m.) > > > Review request for Flume. > > > Bugs: FLUME-2983 > https://issues.apache.org/jira/browse/FLUME-2983 > > > Repository: flume-git > > > Description > ------- > > Similar to FLUME-2972, Offsets tracking the position in Kafka consumers > change from using zookeeper for offset storage to Kafka when moving from > 0.8.x to 0.9.x. > FLUME-2821 makes the client change in the Kafka Source but does not ensure > existing offsets get migrated in order to continue consuming where it left > off. > This patch adds automated logic on startup to check if Kafka offsets exist, > if not and migration is enabled (by default) then copy the offsets from > Zookeeper and commit them to Kafka. > This patch also fixes the backwards incompatibility caused by removing the > zookeeperConnect property. The bootstrap can be looked up if zookeeperConnect > is used. > > > Diffs > ----- > > > flume-ng-channels/flume-kafka-channel/src/main/java/org/apache/flume/channel/kafka/KafkaChannel.java > e7f1f2e > > flume-ng-channels/flume-kafka-channel/src/test/java/org/apache/flume/channel/kafka/TestKafkaChannel.java > e7ae68f > flume-ng-doc/sphinx/FlumeUserGuide.rst 0fd1ec9 > flume-ng-sources/flume-kafka-source/pom.xml 5f5c2a8 > > flume-ng-sources/flume-kafka-source/src/main/java/org/apache/flume/source/kafka/KafkaSource.java > 86782c3 > > flume-ng-sources/flume-kafka-source/src/main/java/org/apache/flume/source/kafka/KafkaSourceConstants.java > 1f255f9 > > flume-ng-sources/flume-kafka-source/src/test/java/org/apache/flume/source/kafka/KafkaSourceEmbeddedKafka.java > a3a2f92 > > flume-ng-sources/flume-kafka-source/src/test/java/org/apache/flume/source/kafka/TestKafkaSource.java > 1598741 > > Diff: https://reviews.apache.org/r/51541/diff/ > > > Testing > ------- > > Unit tests so far. > > > Thanks, > > Grant Henke > >
