----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31967/#review76511 -----------------------------------------------------------
core/src/main/scala/kafka/cluster/Partition.scala <https://reviews.apache.org/r/31967/#comment124067> General comment for changes to this file: The lines are longer than 80 in many cases. Can you please fix that? core/src/main/scala/kafka/cluster/Partition.scala <https://reviews.apache.org/r/31967/#comment124073> scala.Some is unused. core/src/main/scala/kafka/cluster/Partition.scala <https://reviews.apache.org/r/31967/#comment124074> Let's rename FetchDataInfo.fetchOffset to fetchOffsetMetadata. It is confusing to read ...fetchOffset.messageOffset. core/src/main/scala/kafka/cluster/Partition.scala <https://reviews.apache.org/r/31967/#comment124068> Remove space after ! in all relevant places in this patch? core/src/main/scala/kafka/cluster/Partition.scala <https://reviews.apache.org/r/31967/#comment124075> This comment needs an update to match the logic in your patch. core/src/main/scala/kafka/cluster/Replica.scala <https://reviews.apache.org/r/31967/#comment124069> Easier to express clearly the case for UnknownLogReadResult. It is harder to read and convince oneself that the case where the broker becomes a leader doesn't set the lagBeginValue to current time. In addition to adding that condition explicitly, can you also add a comment explaining the significance of the check? core/src/main/scala/kafka/server/KafkaConfig.scala <https://reviews.apache.org/r/31967/#comment124066> minor nit: hasnt => hasn't Also, how about "hasn't consumed up to the leader's log end offset for at least this time" core/src/main/scala/kafka/server/ReplicaManager.scala <https://reviews.apache.org/r/31967/#comment124070> Please break the long lines. Makes it harder to read and maintain the code core/src/main/scala/kafka/server/ReplicaManager.scala <https://reviews.apache.org/r/31967/#comment124071> ditto core/src/main/scala/kafka/server/ReplicaManager.scala <https://reviews.apache.org/r/31967/#comment124072> ditto - Neha Narkhede On March 12, 2015, 8:42 p.m., Aditya Auradkar wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/31967/ > ----------------------------------------------------------- > > (Updated March 12, 2015, 8:42 p.m.) > > > Review request for kafka. > > > Bugs: KAFKA-1546 > https://issues.apache.org/jira/browse/KAFKA-1546 > > > Repository: kafka > > > Description > ------- > > PATCH for KAFKA-1546 > > Brief summary of changes: > - Added a lagBegin metric inside Replica to track the lag in terms of time > since the replica did not read from the LEO > - Using lag begin value in the check for ISR expand and shrink > - Removed the max lag messages config since it is no longer necessary > - Returning the initialLogEndOffset in LogReadResult corresponding to the the > LEO before actually reading from the log. > - Unit test cases to test ISR shrinkage and expansion > > > Diffs > ----- > > core/src/main/scala/kafka/cluster/Partition.scala > c4bf48a801007ebe7497077d2018d6dffe1677d4 > core/src/main/scala/kafka/cluster/Replica.scala > bd13c20338ce3d73113224440e858a12814e5adb > core/src/main/scala/kafka/server/KafkaConfig.scala > 48e33626695ad8a28b0018362ac225f11df94973 > core/src/main/scala/kafka/server/ReplicaManager.scala > c5274822c57bf3c1f9e4135c0bdcaa87ee50ce20 > core/src/test/scala/unit/kafka/server/ISRExpirationTest.scala > 92152358c95fa9178d71bd1c079af0a0bd8f1da8 > core/src/test/scala/unit/kafka/server/KafkaConfigConfigDefTest.scala > c124c8df5b5079e5ffbd0c4ea359562a66aaf317 > core/src/test/scala/unit/kafka/server/LogRecoveryTest.scala > 92d6b2c672f74cdd526f2e98da8f7fb3696a88e3 > core/src/test/scala/unit/kafka/server/SimpleFetchTest.scala > efb457334bd87e4c5b0dd2c66ae1995993cd0bc1 > > Diff: https://reviews.apache.org/r/31967/diff/ > > > Testing > ------- > > > Thanks, > > Aditya Auradkar > >
