Kafka
  1. Kafka
  2. KAFKA-204

BoundedByteBufferReceive hides OutOfMemoryError

    Details

    • Type: Bug Bug
    • Status: Resolved
    • Priority: Critical 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
        Jay Kreps added a comment -

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

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

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

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

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

        Show
        Chris Burroughs added a comment - Tiny patch for this one case, created KAFKA-205 to follow up.
        Hide
        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
        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
        Jun Rao added a comment -

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

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

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

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

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

        Show
        Jun Rao added a comment - Hmm, it doesn't look like OutOfMemoryError allow one to specify a cause during initialization.
        Hide
        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
        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
        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
        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
        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
        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
        Jun Rao added a comment -

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

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

        +1

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

        Just committed this.

        Show
        Jun Rao added a comment - Just committed this.

          People

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

            Dates

            • Created:
              Updated:
              Resolved:

              Development