----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/52598/#review151795 -----------------------------------------------------------
flume-ng-channels/flume-kafka-channel/src/main/java/org/apache/flume/channel/kafka/KafkaChannelConfiguration.java (line 52) <https://reviews.apache.org/r/52598/#comment220313> please see my comments on the docs w.r.t. naming flume-ng-channels/flume-kafka-channel/src/test/java/org/apache/flume/channel/kafka/TestKafkaChannel.java (line 369) <https://reviews.apache.org/r/52598/#comment220318> Please add doc comment flume-ng-channels/flume-kafka-channel/src/test/java/org/apache/flume/channel/kafka/TestKafkaChannel.java (line 399) <https://reviews.apache.org/r/52598/#comment220316> Pls document what the different values mean, i.e. when staticPtn == null what does that mean; also what are the params to this function. flume-ng-channels/flume-kafka-channel/src/test/java/org/apache/flume/channel/kafka/TestKafkaChannel.java (line 400) <https://reviews.apache.org/r/52598/#comment220315> style nit: This should just be a Javadoc comment. However, in comments, for readability, please add punctuation (periods) to your sentences, and also leave a space after the "//" in your comments, like this: // This method tests both the default behavior (usePartitionHeader=false) // and the behaviour when the partitionId setting is used. // Under the default behaviour you would expect an even distribution of // messages to partitions, however when partitionId is used we manually create // a large skew to some partitions and then verify that this actually happened. flume-ng-channels/flume-kafka-channel/src/test/java/org/apache/flume/channel/kafka/TestKafkaChannel.java (line 407) <https://reviews.apache.org/r/52598/#comment220317> If that is the case why not use a test-wide constant for it? flume-ng-channels/flume-kafka-channel/src/test/java/org/apache/flume/channel/kafka/TestKafkaChannel.java (line 430) <https://reviews.apache.org/r/52598/#comment220320> Would you mind factoring this loop into its own helper function, since it seems very self-contained? flume-ng-channels/flume-kafka-channel/src/test/java/org/apache/flume/channel/kafka/TestKafkaChannel.java (line 484) <https://reviews.apache.org/r/52598/#comment220322> mind moving this comment to the top of the for-loop? flume-ng-doc/sphinx/FlumeUserGuide.rst (line 1268) <https://reviews.apache.org/r/52598/#comment220307> I am confused about what this means in the context of the source. 2. 1. Did you really mean to put this in the docs for the source? 2. s/channel/source/ on the first line here ? flume-ng-doc/sphinx/FlumeUserGuide.rst (line 2582) <https://reviews.apache.org/r/52598/#comment220310> defaultPartitionId might be a better name. Also, what happens when neither defaultPartitionId nor the partition ID header are present? Who decides the partition then? (i.e. what is the current behavior). Would be good to document that as well. flume-ng-doc/sphinx/FlumeUserGuide.rst (line 2584) <https://reviews.apache.org/r/52598/#comment220308> Hmm, how about partitionIdHeader flume-ng-doc/sphinx/FlumeUserGuide.rst (line 2586) <https://reviews.apache.org/r/52598/#comment220309> TODO here? An error will be thrown and the event will be rejected from the channel? flume-ng-doc/sphinx/FlumeUserGuide.rst (line 2791) <https://reviews.apache.org/r/52598/#comment220311> same as above flume-ng-doc/sphinx/FlumeUserGuide.rst (line 2793) <https://reviews.apache.org/r/52598/#comment220312> same as above flume-ng-sinks/flume-ng-kafka-sink/src/main/java/org/apache/flume/sink/kafka/KafkaSink.java (line 214) <https://reviews.apache.org/r/52598/#comment220325> nit: missing space after comma. Did you run checkstyle? flume-ng-sinks/flume-ng-kafka-sink/src/main/java/org/apache/flume/sink/kafka/KafkaSink.java (line 216) <https://reviews.apache.org/r/52598/#comment220326> also missing space after comma on this line flume-ng-sinks/flume-ng-kafka-sink/src/main/java/org/apache/flume/sink/kafka/KafkaSink.java (line 221) <https://reviews.apache.org/r/52598/#comment220327> Do you really need to catch Exception? If so, then why bother catching all the rest as well? flume-ng-sinks/flume-ng-kafka-sink/src/test/java/org/apache/flume/sink/kafka/TestKafkaSink.java (line 391) <https://reviews.apache.org/r/52598/#comment220328> Is this just copy / paste from the other one? Can we find a way to make them share code? flume-ng-sinks/flume-ng-kafka-sink/src/test/java/org/apache/flume/sink/kafka/util/KafkaConsumer.java (line 55) <https://reviews.apache.org/r/52598/#comment220329> hmm, does this change our user contract or not really? - Mike Percy On Oct. 6, 2016, 5:13 a.m., Tristan Stevens wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/52598/ > ----------------------------------------------------------- > > (Updated Oct. 6, 2016, 5:13 a.m.) > > > Review request for Flume and Grant Henke. > > > Repository: flume-git > > > Description > ------- > > This feature is useful for anyone who needs greater control of which > partitions are being written to - normally in a situation where multiple > Flume agents are being deployed in order to horizontally scale, or > alternatively if there is a scenario where there is a skew in data that might > lead to one or more partitions hotspotting. > We also have the ability to specify custom partitions on to the Kafka > Producer itself using the kafka.* configuration properties. > > The Kafka Producer provides the ability to set the partition ID using the > following constructor > (https://kafka.apache.org/090/javadoc/org/apache/kafka/clients/producer/ProducerRecord.html#ProducerRecord%28java.lang.String,%20java.lang.Integer,%20K,%20V%29 > ), this is just a matter of providing the option to use this constructor. > > This is specified in one of two ways: either via the staticPartition > configuration property, which means that every message goes to the specified > partition, or via the partitionHeader configuration property, which directs > the implementation to retrieve the partitionId from one of the event headers. > > > Diffs > ----- > > > flume-ng-channels/flume-kafka-channel/src/main/java/org/apache/flume/channel/kafka/KafkaChannel.java > 66b553a > > flume-ng-channels/flume-kafka-channel/src/main/java/org/apache/flume/channel/kafka/KafkaChannelConfiguration.java > 3ab807b > > flume-ng-channels/flume-kafka-channel/src/test/java/org/apache/flume/channel/kafka/TestKafkaChannel.java > 57c0b28 > flume-ng-doc/sphinx/FlumeUserGuide.rst ab71d38 > > flume-ng-sinks/flume-ng-kafka-sink/src/main/java/org/apache/flume/sink/kafka/KafkaSink.java > 89bdd84 > > flume-ng-sinks/flume-ng-kafka-sink/src/main/java/org/apache/flume/sink/kafka/KafkaSinkConstants.java > 1bf380c > > flume-ng-sinks/flume-ng-kafka-sink/src/test/java/org/apache/flume/sink/kafka/TestKafkaSink.java > 76eca37 > > flume-ng-sinks/flume-ng-kafka-sink/src/test/java/org/apache/flume/sink/kafka/util/KafkaConsumer.java > d5dfbd6 > > Diff: https://reviews.apache.org/r/52598/diff/ > > > Testing > ------- > > Unit testing done for both Kafka Channel and Kafka Sink. > > > Thanks, > > Tristan Stevens > >
