> On Aug. 31, 2016, 3:27 p.m., Denes Arvay wrote: > > flume-ng-sources/flume-kafka-source/src/main/java/org/apache/flume/source/kafka/KafkaSource.java, > > lines 538-606 > > <https://reviews.apache.org/r/51541/diff/1/?file=1488879#file1488879line538> > > > > 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.
I agree. I felt the same way initially. The reasons I didn't break it out into a utility class were: - It's not clear the correct place it would go - This code is temporary and should be removed when migration support can be dropped - Though very similar there are some differences in the implimentation I can open a jira to track consolidation. > On Aug. 31, 2016, 3:27 p.m., Denes Arvay wrote: > > flume-ng-sources/flume-kafka-source/src/main/java/org/apache/flume/source/kafka/KafkaSource.java, > > line 372 > > <https://reviews.apache.org/r/51541/diff/1/?file=1488879#file1488879line372> > > > > 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. There are a ton of places this pattern is used so I purposly didn't deviate from that for consistency. I think you could change this pattern everywhere in a sepearate re-factoring/clean up jira if you like. > On Aug. 31, 2016, 3:27 p.m., Denes Arvay wrote: > > flume-ng-sources/flume-kafka-source/src/main/java/org/apache/flume/source/kafka/KafkaSource.java, > > line 394 > > <https://reviews.apache.org/r/51541/diff/1/?file=1488879#file1488879line394> > > > > 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? I actually didn't change this logic, I just moved it out of setConsumerProps. I think finding and work done on this can be tracked in FLUME-2857 and other jiras. - Grant ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/51541/#review147430 ----------------------------------------------------------- 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 > >
