Home | About | Sematext search-lucene.com search-hadoop.com
 Search Hadoop and all its subprojects:

Switch to Threaded View
Kafka, mail # dev - Review Request 21588: Fix KAFKA-1430


Copy link to this message
-
Re: Review Request 21588: Fix KAFKA-1430, round two
Jun Rao 2014-06-07, 02:01

This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/21588/#review44998
Overall, the patch looks pretty good to me. Some comments below.
core/src/main/scala/kafka/api/FetchResponse.scala
<https://reviews.apache.org/r/21588/#comment79629>

    There is no need to specify val explicitly. Case class automatically makes every parameter in the constructor a val.

core/src/main/scala/kafka/api/ProducerResponse.scala
<https://reviews.apache.org/r/21588/#comment79630>

    Ditto as the above.

core/src/main/scala/kafka/log/FileMessageSet.scala
<https://reviews.apache.org/r/21588/#comment79633>

    Should this be a val instead of a function?

core/src/main/scala/kafka/log/Log.scala
<https://reviews.apache.org/r/21588/#comment79634>

    Does time need to be val?

core/src/main/scala/kafka/log/Log.scala
<https://reviews.apache.org/r/21588/#comment79635>

    Hmm, when we get an empty ByteBufferMessage, it may be important to return the corresponding offsetMetadata.

core/src/main/scala/kafka/log/Log.scala
<https://reviews.apache.org/r/21588/#comment79636>

    This can only happen to regular consumer fetch requests. So, returning an UnknowOffset is ok since it's not going to be used for purgatory checking. We should describe this in the comment.

core/src/main/scala/kafka/log/Log.scala
<https://reviews.apache.org/r/21588/#comment79637>

    Perhaps this can be named as convertToOffsetMetadata()?

core/src/main/scala/kafka/server/KafkaServer.scala
<https://reviews.apache.org/r/21588/#comment79640>

    Could this be done inside KafkaApis?

core/src/main/scala/kafka/server/LogOffsetMetadata.scala
<https://reviews.apache.org/r/21588/#comment79642>

    Could we make this a case class? Then equal() doesn't need to be overwritten.

core/src/main/scala/kafka/server/LogOffsetMetadata.scala
<https://reviews.apache.org/r/21588/#comment79641>

    older() and peer() could be named better. How about offsetOnOlderSegment and offsetOnSameSegment?

core/src/main/scala/kafka/server/LogOffsetMetadata.scala
<https://reviews.apache.org/r/21588/#comment79643>

    Maybe postitionDiff()?

core/src/main/scala/kafka/server/RequestPurgatory.scala
<https://reviews.apache.org/r/21588/#comment79644>

    Do we need to change to override val?

core/src/main/scala/kafka/server/RequestPurgatory.scala
<https://reviews.apache.org/r/21588/#comment79645>

    We will need to sync on t when doing the check (same as in line 187) to avoid the race condition that can cause two responses to be sent for the same request.
- Jun Rao
On June 6, 2014, 12:41 a.m., Guozhang Wang wrote: