> On Sept. 1, 2016, 12:32 a.m., Mike Percy wrote: > > not very happy about the copy/paste but this is an important fix. we should > > clean it up later though. just a few comments
Yeah, me either. There was no common module that existed to hold this code and I didn't want to add one just for this. I also didn't want to add Kafka dependencies to a module that didn't have them already in this change. There is a jira tracking future consolidation. > On Sept. 1, 2016, 12:32 a.m., Mike Percy wrote: > > flume-ng-sources/flume-kafka-source/src/test/java/org/apache/flume/source/kafka/KafkaSourceEmbeddedKafka.java, > > line 39 > > <https://reviews.apache.org/r/51541/diff/2/?file=1489155#file1489155line39> > > > > Why this change? Kafka uses/advertises the hostname, not the ip address. When I test deriving the bootstrap from Zookeeper, I compare with this host to validate it is generated correctly. In order to use it for comparison it should be the hostname. > On Sept. 1, 2016, 12:32 a.m., Mike Percy wrote: > > flume-ng-sources/flume-kafka-source/src/test/java/org/apache/flume/source/kafka/KafkaSourceEmbeddedKafka.java, > > line 83 > > <https://reviews.apache.org/r/51541/diff/2/?file=1489155#file1489155line83> > > > > I liked the old method name better... j/k Will change back. ;) - Grant ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/51541/#review147505 ----------------------------------------------------------- On Sept. 1, 2016, 1:17 a.m., Grant Henke wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/51541/ > ----------------------------------------------------------- > > (Updated Sept. 1, 2016, 1:17 a.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 > >
