----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/40624/#review107823 -----------------------------------------------------------
Ship it! Overall, looks good. Some minor nits and question. Clarify question, Fix it and then Ship it! :) docs/learn/documentation/versioned/jobs/configuration-table.html (line 884) <https://reviews.apache.org/r/40624/#comment167131> Should be: *systems.system-name.samza.fetch.threshold parameter* determines the number of messages ... *This parameter* defines how many bytes to use ... docs/learn/documentation/versioned/jobs/configuration-table.html (line 888) <https://reviews.apache.org/r/40624/#comment167132> nit: fetches *entire* messages, hence this ... docs/learn/documentation/versioned/jobs/configuration-table.html (line 891) <https://reviews.apache.org/r/40624/#comment167133> Could use a line break here nit: For example, if *this parameter* is set to ... samza-api/src/main/java/org/apache/samza/system/IncomingMessageEnvelope.java (line 37) <https://reviews.apache.org/r/40624/#comment167135> Prefer annotation here saying @VisibleForTesting What is the reasoning behind - 4 + 64 + 4 + 4 + 4? samza-api/src/main/java/org/apache/samza/util/BlockingEnvelopeMap.java (line 77) <https://reviews.apache.org/r/40624/#comment167146> nit: method not used anywhere. We can remove it. samza-kafka/src/main/scala/org/apache/samza/system/kafka/KafkaSystemConsumer.scala (line 123) <https://reviews.apache.org/r/40624/#comment167141> nit: adjust indentation samza-kafka/src/main/scala/org/apache/samza/system/kafka/KafkaSystemFactory.scala (line 82) <https://reviews.apache.org/r/40624/#comment167143> Prefer to move this condition to KafkaConfig as a method - boolean isFetchLimitByBytesEnabled(); - Navina Ramesh On Nov. 24, 2015, 8 p.m., Yi Pan (Data Infrastructure) wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/40624/ > ----------------------------------------------------------- > > (Updated Nov. 24, 2015, 8 p.m.) > > > Review request for samza, Monal Daxini and Navina Ramesh. > > > Bugs: SAMZA-775 > https://issues.apache.org/jira/browse/SAMZA-775 > > > Repository: samza > > > Description > ------- > > Ported the patch to 0.10 > > > Diffs > ----- > > docs/learn/documentation/versioned/jobs/configuration-table.html > b5d3813ab44062f61cb33e4b37bb7548d8ff0617 > > samza-api/src/main/java/org/apache/samza/system/IncomingMessageEnvelope.java > 4b143124d28308751f6725278d5afe57584c0d33 > samza-api/src/main/java/org/apache/samza/util/BlockingEnvelopeMap.java > 7dd99fb4ab435397f74644b71a4a56b9969dcde5 > samza-api/src/test/java/org/apache/samza/util/TestBlockingEnvelopeMap.java > d1a0a82d29eeef1136007f0519581f711c026b07 > samza-kafka/src/main/scala/org/apache/samza/config/KafkaConfig.scala > a65e8e8f2c3c526fa11d269ef57b1abe68ec5bc6 > > samza-kafka/src/main/scala/org/apache/samza/system/kafka/KafkaSystemConsumer.scala > c948d6405f845377d16af4411a243a8cff66e81d > > samza-kafka/src/main/scala/org/apache/samza/system/kafka/KafkaSystemFactory.scala > a60cda206130e219da8a8ba5421f2d7a45de4a81 > samza-kafka/src/test/scala/org/apache/samza/config/TestKafkaConfig.scala > 85badf9f1a8fcbf0f478c30195c59a9bbe64feb6 > > samza-kafka/src/test/scala/org/apache/samza/system/kafka/TestKafkaSystemConsumer.scala > 23fa9398e89edeeabfd3ee754c27a8f3bec417b0 > > Diff: https://reviews.apache.org/r/40624/diff/ > > > Testing > ------- > > ./gradlew clean build passed. > run ./bin/integration-tests.sh passed as well. > > > Thanks, > > Yi Pan (Data Infrastructure) > >
