----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/16706/#review31324 -----------------------------------------------------------
samza-kafka/src/main/scala/org/apache/samza/config/KafkaConfig.scala <https://reviews.apache.org/r/16706/#comment59789> Recommend keeping these settings identical with Kafka's config naming scheme: systems.%s.consumer.fetch.size.bytes to systems.%s.consumer.fetch.message.max.bytes systems.%s.consumer.min.size.bytes to systems.%s.consumer.fetch.min.bytes systems.%s.consumer.max.wait.ms to systems.%s.consumer.fetch.wait.max.ms samza-kafka/src/main/scala/org/apache/samza/system/kafka/BrokerProxy.scala <https://reviews.apache.org/r/16706/#comment59790> Recommend switching all 5 of these defaults to just use Kafka's ConsumerConfig constants directly: https://github.com/apache/kafka/blob/trunk/core/src/main/scala/kafka/consumer/ConsumerConfig.scala Would also make the change to timeout in KafkaSystemConsumer's constructor, as well. samza-kafka/src/main/scala/org/apache/samza/system/kafka/KafkaSystemConsumer.scala <https://reviews.apache.org/r/16706/#comment59791> See comment above about using ConsumerConfig constants instead. samza-kafka/src/main/scala/org/apache/samza/system/kafka/KafkaSystemFactory.scala <https://reviews.apache.org/r/16706/#comment59793> I think you should use: consumerConfig.fetchMessageMaxBytes consumerConfig.fetchMinBytes consumerConfig.fetchWaitMaxMs The way we do for the rest of the variables. samza-kafka/src/main/scala/org/apache/samza/system/kafka/KafkaSystemFactory.scala <https://reviews.apache.org/r/16706/#comment59794> This is really weird... any idea why these guys are here? Seems like deleting it makes sense. Wonder if it was a copy/paste error on my part. - Chris Riccomini On Jan. 7, 2014, 11:19 p.m., Jakob Homan wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/16706/ > ----------------------------------------------------------- > > (Updated Jan. 7, 2014, 11:19 p.m.) > > > Review request for samza. > > > Bugs: SAMZA-21 > https://issues.apache.org/jira/browse/SAMZA-21 > > > Repository: samza > > > Description > ------- > > samza-21 > > > Diffs > ----- > > samza-kafka/src/main/scala/org/apache/samza/config/KafkaConfig.scala > 978620a > samza-kafka/src/main/scala/org/apache/samza/system/kafka/BrokerProxy.scala > 53b2e22 > > samza-kafka/src/main/scala/org/apache/samza/system/kafka/DefaultFetchSimpleConsumer.scala > 80ea3ea > > samza-kafka/src/main/scala/org/apache/samza/system/kafka/KafkaSystemConsumer.scala > 5dbcd94 > > samza-kafka/src/main/scala/org/apache/samza/system/kafka/KafkaSystemFactory.scala > a11a72a > > samza-kafka/src/test/scala/org/apache/samza/system/kafka/TestBrokerProxy.scala > 9a3a29e > > Diff: https://reviews.apache.org/r/16706/diff/ > > > Testing > ------- > > unit and manual > > > Thanks, > > Jakob Homan > >
