Kafka
  1. Kafka
  2. KAFKA-384

Fix intermittent test failures and remove unnecessary sleeps

    Details

    • Type: Bug Bug
    • Status: Resolved
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 0.8.0
    • Fix Version/s: None
    • Component/s: None
    • Labels:
      None

      Description

      Seeing intermittent failures in 0.8 unit tests. Also, many sleeps can be removed (with producer acks in place) and I think MockTime isn't used in some places where it should.

      1. kafka-384-v2.patch
        37 kB
        Neha Narkhede
      2. kafka-384-v1.patch
        37 kB
        Neha Narkhede

        Activity

        Hide
        Neha Narkhede added a comment - - edited

        This patch removes sleep statements from the unit tests. Changes include -

        1. All sleep statements except those related to a scheduler are removed. So 2-3 sleep statements that exercise the producer side queue expiration logic have not been removed
        2. For the Log tests, passed in a Time parameter to the Log. Kafka server already takes in an optional Time parameter that defaults to SystemTime. However, it wasn't passed around in LogManager. Fixed it so KafkaServer passes its Time
        variable to LogManager which passes it to Log.
        This is useful in removing all sleep statements from unit tests for the log manager and logs

        Show
        Neha Narkhede added a comment - - edited This patch removes sleep statements from the unit tests. Changes include - 1. All sleep statements except those related to a scheduler are removed. So 2-3 sleep statements that exercise the producer side queue expiration logic have not been removed 2. For the Log tests, passed in a Time parameter to the Log. Kafka server already takes in an optional Time parameter that defaults to SystemTime. However, it wasn't passed around in LogManager. Fixed it so KafkaServer passes its Time variable to LogManager which passes it to Log. This is useful in removing all sleep statements from unit tests for the log manager and logs
        Hide
        Jun Rao added a comment -

        Thanks for the patch. It looks good. Just 1 comment:

        1. AsyncProducerTest.testQueueTimeExpired(): This is an existing issue, but probably can be fixed in this patch too. It seems that we should do producerSendThread.shutdown after EasyMock.verify(mockHandler). Shutdown always sends all remaining messages in the buffer. If we call shutdown before verify, it's not clear if the send was triggered by timeout or shutdown.

        Show
        Jun Rao added a comment - Thanks for the patch. It looks good. Just 1 comment: 1. AsyncProducerTest.testQueueTimeExpired(): This is an existing issue, but probably can be fixed in this patch too. It seems that we should do producerSendThread.shutdown after EasyMock.verify(mockHandler). Shutdown always sends all remaining messages in the buffer. If we call shutdown before verify, it's not clear if the send was triggered by timeout or shutdown.
        Hide
        Neha Narkhede added a comment -

        Thanks for the review, Jun. That is a good point. I fixed that and also removed the reference to mockTime since that is not useful here.

        Show
        Neha Narkhede added a comment - Thanks for the review, Jun. That is a good point. I fixed that and also removed the reference to mockTime since that is not useful here.
        Hide
        Joel Koshy added a comment -

        +1 for v2.

        Show
        Joel Koshy added a comment - +1 for v2.
        Hide
        Jay Kreps added a comment -

        You are my hero...

        Show
        Jay Kreps added a comment - You are my hero...
        Hide
        Jun Rao added a comment -

        +1 on v2 too.

        Show
        Jun Rao added a comment - +1 on v2 too.
        Hide
        Neha Narkhede added a comment -

        KAFKA-343 checkin broke some unit tests and cause others to hang. I think I might have to hold off on the checkin until that is either fixed or reverted.

        Show
        Neha Narkhede added a comment - KAFKA-343 checkin broke some unit tests and cause others to hang. I think I might have to hold off on the checkin until that is either fixed or reverted.
        Hide
        Neha Narkhede added a comment -

        Thanks all for the review! Committed the v2 patch.

        Show
        Neha Narkhede added a comment - Thanks all for the review! Committed the v2 patch.

          People

          • Assignee:
            Neha Narkhede
            Reporter:
            Joel Koshy
          • Votes:
            0 Vote for this issue
            Watchers:
            4 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development