Uploaded image for project: 'Kafka'
  1. Kafka
  2. KAFKA-1836

metadata.fetch.timeout.ms set to zero blocks forever

Details

    • Bug
    • Status: Resolved
    • Minor
    • Resolution: Fixed
    • 0.8.2.0
    • 0.9.0.0
    • clients

    Description

      You can easily work around this by setting the timeout value to 1ms, but 0ms should mean 0ms or at least have the behavior documented.

      Attachments

        1. KAFKA-1836.patch
          9 kB
          Jaikiran Pai
        2. KAFKA-1836-new-patch.patch
          6 kB
          Jaikiran Pai

        Activity

          jkreps Jay Kreps added a comment -

          Looks like Metadata.awaitUpdate() always calls Object.wait(maxWaitMs) even if maxWaitMs is 0. Unfortunately Object.wait(0) doesn't mean wait for zero ms, it means wait forever. So it should be an easy fix--we need an if statement here to check for a wait time of 0.

          jkreps Jay Kreps added a comment - Looks like Metadata.awaitUpdate() always calls Object.wait(maxWaitMs) even if maxWaitMs is 0. Unfortunately Object.wait(0) doesn't mean wait for zero ms, it means wait forever. So it should be an easy fix--we need an if statement here to check for a wait time of 0.
          jaikiran Jaikiran Pai added a comment -

          I've attached a patch which handles <= 0 values in the awaitUpdate method. I've also added a testcase to verify this change. Can you take a look at this and see if this makes sense?

          jaikiran Jaikiran Pai added a comment - I've attached a patch which handles <= 0 values in the awaitUpdate method. I've also added a testcase to verify this change. Can you take a look at this and see if this makes sense?

          A couple of notes on the current patch:

          • I don't think we need to change the config limits – 0 should be an acceptable setting, it just isn't behaving properly right now.
          • You can get the right behavior just by adding an if statement as Jay suggested – don't call wait() if maxWaitMs is 0. The subsequent check that generates the TimeoutException should then work.
          • Checking for the invalid value < 0 is ok, but the code that calls it guarantees it won't since the config is already validated to have a value >= 0.
          • In the test, maybe check that the timeout is correct rather than whether you got the right metadata back since the test is supposed to focus on the timeouts.
          ewencp Ewen Cheslack-Postava added a comment - A couple of notes on the current patch: I don't think we need to change the config limits – 0 should be an acceptable setting, it just isn't behaving properly right now. You can get the right behavior just by adding an if statement as Jay suggested – don't call wait() if maxWaitMs is 0. The subsequent check that generates the TimeoutException should then work. Checking for the invalid value < 0 is ok, but the code that calls it guarantees it won't since the config is already validated to have a value >= 0. In the test, maybe check that the timeout is correct rather than whether you got the right metadata back since the test is supposed to focus on the timeouts.
          jaikiran Jaikiran Pai added a comment -

          Thanks Ewen for your feedback. I've taken into account your comments and updated the patch accordingly and created a review request https://reviews.apache.org/r/29752/. I've also uploaded the same patch here to the JIRA.

          jaikiran Jaikiran Pai added a comment - Thanks Ewen for your feedback. I've taken into account your comments and updated the patch accordingly and created a review request https://reviews.apache.org/r/29752/ . I've also uploaded the same patch here to the JIRA.
          nehanarkhede Neha Narkhede added a comment -

          jaikiran Will help you check this in after ewencp's comment is addressed.

          nehanarkhede Neha Narkhede added a comment - jaikiran Will help you check this in after ewencp 's comment is addressed.
          jaikiran Jaikiran Pai added a comment -

          Thanks nehanarkhede, I've updated the review board patch to address ewencp's latest comment. Thanks ewencp for reviewing it.

          jaikiran Jaikiran Pai added a comment - Thanks nehanarkhede , I've updated the review board patch to address ewencp 's latest comment. Thanks ewencp for reviewing it.
          nehanarkhede Neha Narkhede added a comment -

          Thanks for the patches! Pushed to trunk

          nehanarkhede Neha Narkhede added a comment - Thanks for the patches! Pushed to trunk

          People

            jaikiran Jaikiran Pai
            ppearcy Paul Pearcy
            Neha Narkhede Neha Narkhede
            Votes:
            0 Vote for this issue
            Watchers:
            6 Start watching this issue

            Dates

              Created:
              Updated:
              Resolved: