Kafka
  1. Kafka
  2. KAFKA-823

merge 0.8 (51421fcc0111031bb77f779a6f6c00520d526a34) to trunk

    Details

    • Type: Task Task
    • Status: Resolved
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 0.8.1
    • Component/s: core
    • Labels:
      None

      Description

      merge 0.8 up to the following commit to trunk

      commit 51421fcc0111031bb77f779a6f6c00520d526a34
      Author: Neha Narkhede <neha.narkhede@gmail.com>
      Date: Fri Mar 22 09:32:27 2013 -0700

      KAFKA-816 Reduce noise in Kafka server logs due to NotLeaderForPartitionException; reviewed by Jun Rao

      1. kafka-823_v4.patch
        326 kB
        Jun Rao
      2. kafka-823_v3.patch
        331 kB
        Jun Rao
      3. kafka-823_v2.patch
        36 kB
        Jun Rao
      4. kafka-823.patch
        36 kB
        Jun Rao

        Activity

        Hide
        Jun Rao added a comment -

        Thanks for the review. Committed to trunk.

        Show
        Jun Rao added a comment - Thanks for the review. Committed to trunk.
        Hide
        Jay Kreps added a comment -

        +1 This looks great, thanks for taking the extra time.

        Show
        Jay Kreps added a comment - +1 This looks great, thanks for taking the extra time.
        Hide
        Jun Rao added a comment -

        Thanks for the review. The suggestions make sense. Attach patch v4 that addresses those issues. The implication is that now the flush interval is based on # of shallow messages, instead of deep. We can revisit that separately.

        Show
        Jun Rao added a comment - Thanks for the review. The suggestions make sense. Attach patch v4 that addresses those issues. The implication is that now the flush interval is based on # of shallow messages, instead of deep. We can revisit that separately.
        Hide
        Jay Kreps added a comment -

        Comments:

        • I think moving Log.append to return a tuple of longs is a worse api as it won't support extension in the future. This is the spot where we do our iteration of the message set so it is likely that we will have other stats we want to compute. Let's keep the append info object.
        • If there are wrong values in the append info object they need to be fixed or renamed (shallowCount?) we can't just leave them there wrong
        • You moved back to having the if(assignOffsets) block return the last offset. How come?
        • In general I think the logic of using (lastOffset - firstOffset + 1) as the number of appended messages isn't correct. I think using the shallow message count would be better or passing back the actual number of messages from the deep iteration which would require some refactoring. The problem with using this is that when bootstrapping a follower it would flush on every append for any topic with sparse offsets.
        Show
        Jay Kreps added a comment - Comments: I think moving Log.append to return a tuple of longs is a worse api as it won't support extension in the future. This is the spot where we do our iteration of the message set so it is likely that we will have other stats we want to compute. Let's keep the append info object. If there are wrong values in the append info object they need to be fixed or renamed (shallowCount?) we can't just leave them there wrong You moved back to having the if(assignOffsets) block return the last offset. How come? In general I think the logic of using (lastOffset - firstOffset + 1) as the number of appended messages isn't correct. I think using the shallow message count would be better or passing back the actual number of messages from the deep iteration which would require some refactoring. The problem with using this is that when bootstrapping a follower it would flush on every append for any topic with sparse offsets.
        Hide
        Jun Rao added a comment -

        Attach patch v3. This should apply cleanly to trunk.

        Show
        Jun Rao added a comment - Attach patch v3. This should apply cleanly to trunk.
        Hide
        Jay Kreps added a comment -

        I think the second version of the patch is broken. It doesn't apply cleanly and looks like maybe it is reversed.

        Show
        Jay Kreps added a comment - I think the second version of the patch is broken. It doesn't apply cleanly and looks like maybe it is reversed.
        Hide
        Jun Rao added a comment -

        Attach patch v2. It fixes a problem with counting the number of appended messages from the return value of log.append().

        Log.append() now only returns (firstOffset, lastOffset), which provides enough information for all callers.

        Show
        Jun Rao added a comment - Attach patch v2. It fixes a problem with counting the number of appended messages from the return value of log.append(). Log.append() now only returns (firstOffset, lastOffset), which provides enough information for all callers.
        Hide
        Jun Rao added a comment -

        Verified that unit tests pass.

        Show
        Jun Rao added a comment - Verified that unit tests pass.
        Hide
        Jun Rao added a comment -

        Attach a patch. The following 7 files have conflicts.

        config/log4j.properties
        core/src/main/scala/kafka/log/Log.scala
        core/src/main/scala/kafka/server/ReplicaFetcherThread.scala
        core/src/main/scala/kafka/server/ReplicaManager.scala
        core/src/main/scala/kafka/tools/DumpLogSegments.scala
        core/src/main/scala/kafka/tools/SimpleConsumerShell.scala
        core/src/test/scala/unit/kafka/log/LogTest.scala

        The only file that needs non-trivial manual merge is log.scala. Please review.

        Show
        Jun Rao added a comment - Attach a patch. The following 7 files have conflicts. config/log4j.properties core/src/main/scala/kafka/log/Log.scala core/src/main/scala/kafka/server/ReplicaFetcherThread.scala core/src/main/scala/kafka/server/ReplicaManager.scala core/src/main/scala/kafka/tools/DumpLogSegments.scala core/src/main/scala/kafka/tools/SimpleConsumerShell.scala core/src/test/scala/unit/kafka/log/LogTest.scala The only file that needs non-trivial manual merge is log.scala. Please review.

          People

          • Assignee:
            Jun Rao
            Reporter:
            Jun Rao
          • Votes:
            0 Vote for this issue
            Watchers:
            2 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development