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

BoundedByteBufferReceive hides OutOfMemoryError

    Details

    • Type: Bug
    • Status: Resolved
    • Priority: Critical
    • Resolution: Fixed
    • Affects Version/s: 0.7
    • Fix Version/s: 0.7.1
    • Component/s: None
    • Labels:
      None

      Description

      private def byteBufferAllocate(size: Int): ByteBuffer = {
      var buffer: ByteBuffer = null
      try

      { buffer = ByteBuffer.allocate(size) }

      catch

      { case e: OutOfMemoryError => throw new RuntimeException("OOME with size " + size, e) case e2 => throw e2 }

      buffer
      }

      This hides the fact that an Error occurred, and will likely result in some log handler printing a message, instead of exiting with non-zero status. Knowing how large the allocation was that caused an OOM is really nice, so I'd suggest logging in byteBufferAllocate and then re-throwing OutOfMemoryError

      1. k204-v1.txt
        0.7 kB
        Chris Burroughs

        Activity

        Hide
        jkreps Jay Kreps added a comment -

        Uh...does anyone know what the motivation of this was originally? Catching OutOfMemoryError is a bit unorthodox...

        Show
        jkreps Jay Kreps added a comment - Uh...does anyone know what the motivation of this was originally? Catching OutOfMemoryError is a bit unorthodox...
        Hide
        tgautier Taylor Gautier added a comment -

        Agree with Jay - if you get an OOME all bets are off. Best to just exit.

        Show
        tgautier Taylor Gautier added a comment - Agree with Jay - if you get an OOME all bets are off. Best to just exit.
        Hide
        cburroughs Chris Burroughs added a comment -

        Tiny patch for this one case, created KAFKA-205 to follow up.

        Show
        cburroughs Chris Burroughs added a comment - Tiny patch for this one case, created KAFKA-205 to follow up.
        Hide
        junrao Jun Rao added a comment -

        The main reason this was added is to show the requested size and the caller that triggered such a request. It would be nice if both pieces are logged together. With the new patch, those two pieces are logged separately (although should be close) and someone has to link them together manually.

        Show
        junrao Jun Rao added a comment - The main reason this was added is to show the requested size and the caller that triggered such a request. It would be nice if both pieces are logged together. With the new patch, those two pieces are logged separately (although should be close) and someone has to link them together manually.
        Hide
        junrao Jun Rao added a comment -

        Another possibility is to rethrow a new RuntimeException with OOME wrapped as the cause.

        Show
        junrao Jun Rao added a comment - Another possibility is to rethrow a new RuntimeException with OOME wrapped as the cause.
        Hide
        erikvanoosten Erik van Oosten added a comment -

        If you rethrow, then rethrow a new OOME with the original OOME wrapped.

        Show
        erikvanoosten Erik van Oosten added a comment - If you rethrow, then rethrow a new OOME with the original OOME wrapped.
        Hide
        junrao Jun Rao added a comment -

        Hmm, it doesn't look like OutOfMemoryError allow one to specify a cause during initialization.

        Show
        junrao Jun Rao added a comment - Hmm, it doesn't look like OutOfMemoryError allow one to specify a cause during initialization.
        Hide
        jkreps Jay Kreps added a comment -

        I understand the intent, but the important thing here is not to swallow the OOM exception, right? I mean once you hit that all bets are off, you need to restart your process...basically I think we shouldn't be messing with that.

        Show
        jkreps Jay Kreps added a comment - I understand the intent, but the important thing here is not to swallow the OOM exception, right? I mean once you hit that all bets are off, you need to restart your process...basically I think we shouldn't be messing with that.
        Hide
        jkreps Jay Kreps added a comment -

        We have request limits in the server and consumer that should provide protection against this so i think that is the appropriate way to handle it. If it does happen I think we should just break everything and then the person running things should set the configs correctly to limit the max request size the server will accept and the max fetch size for the client.

        Show
        jkreps Jay Kreps added a comment - We have request limits in the server and consumer that should provide protection against this so i think that is the appropriate way to handle it. If it does happen I think we should just break everything and then the person running things should set the configs correctly to limit the max request size the server will accept and the max fetch size for the client.
        Hide
        cburroughs Chris Burroughs added a comment -

        > I mean once you hit that all bets are off, you need to restart your process...basically I think we shouldn't be messing with that.

        Yeah, the most important thing to do is get out of the way and let the process exit with a non-zero status code.

        So the options as I see it are:
        (1) Do something ugly (like pass the original fetch request to byteBufferAllocate) for the purposes of a valiant but possible futile logging attempt (there is no guarantee we will be able to allocate the logging Strings we are already asking for, everything we ad makes that less likely).
        (2) Just rethrow e after a logging attempt in byteBufferAllocate.

        My preference is (2), but if someone prefers (1) that's a reasonable trade off.

        Show
        cburroughs Chris Burroughs added a comment - > I mean once you hit that all bets are off, you need to restart your process...basically I think we shouldn't be messing with that. Yeah, the most important thing to do is get out of the way and let the process exit with a non-zero status code. So the options as I see it are: (1) Do something ugly (like pass the original fetch request to byteBufferAllocate) for the purposes of a valiant but possible futile logging attempt (there is no guarantee we will be able to allocate the logging Strings we are already asking for, everything we ad makes that less likely). (2) Just rethrow e after a logging attempt in byteBufferAllocate. My preference is (2), but if someone prefers (1) that's a reasonable trade off.
        Hide
        junrao Jun Rao added a comment -

        Ok, I am fine with the patch then. Any objection to commit it?

        Show
        junrao Jun Rao added a comment - Ok, I am fine with the patch then. Any objection to commit it?
        Hide
        nehanarkhede Neha Narkhede added a comment -

        +1

        Show
        nehanarkhede Neha Narkhede added a comment - +1
        Hide
        junrao Jun Rao added a comment -

        Just committed this.

        Show
        junrao Jun Rao added a comment - Just committed this.

          People

          • Assignee:
            cburroughs Chris Burroughs
            Reporter:
            cburroughs Chris Burroughs
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development