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

        Transition Time In Source Status Execution Times Last Executer Last Execution Date
        Open Open In Progress In Progress
        4m 11s 1 Jun Rao 23/Mar/13 01:28
        In Progress In Progress Patch Available Patch Available
        17s 1 Jun Rao 23/Mar/13 01:28
        Patch Available Patch Available Resolved Resolved
        26d 3h 25m 1 Jun Rao 18/Apr/13 05:54
        Jun Rao made changes -
        Status Patch Available [ 10002 ] Resolved [ 5 ]
        Fix Version/s 0.8.1 [ 12322960 ]
        Resolution Fixed [ 1 ]
        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.
        Jun Rao made changes -
        Attachment kafka-823_v4.patch [ 12579070 ]
        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.
        Jun Rao made changes -
        Attachment kafka-823_v3.patch [ 12578328 ]
        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.
        Jun Rao made changes -
        Attachment kafka-823_v2.patch [ 12576151 ]
        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.
        Jun Rao made changes -
        Status In Progress [ 3 ] Patch Available [ 10002 ]
        Jun Rao made changes -
        Status Open [ 1 ] In Progress [ 3 ]
        Jun Rao made changes -
        Field Original Value New Value
        Attachment kafka-823.patch [ 12575144 ]
        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.
        Jun Rao created issue -

          People

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

            Dates

            • Created:
              Updated:
              Resolved:

              Development