Details

    • Hadoop Flags:
      Reviewed

      Description

      GetJournalEditServlet has the same problem as that of GetImageServlet (HDFS-3330). It should be fixed in the same way. Also need to make CheckpointFaultInjector visible for journal service tests.

      1. HDFS-3388.HDFS-3092.patch
        8 kB
        Brandon Li
      2. HDFS-3388.HDFS-3092.patch
        7 kB
        Brandon Li
      3. HDFS-3388.HDFS-3092.patch
        6 kB
        Brandon Li

        Issue Links

          Activity

          Hide
          Tsz Wo Nicholas Sze added a comment -

          I have committed this. Thanks, Brandon!

          Show
          Tsz Wo Nicholas Sze added a comment - I have committed this. Thanks, Brandon!
          Hide
          Tsz Wo Nicholas Sze added a comment -

          +1 patch looks good.

          (revised the summary.)

          Show
          Tsz Wo Nicholas Sze added a comment - +1 patch looks good. (revised the summary.)
          Hide
          Brandon Li added a comment -

          The new patch addressed Nicholas' comments.

          Show
          Brandon Li added a comment - The new patch addressed Nicholas' comments.
          Hide
          Tsz Wo Nicholas Sze added a comment -
          • Since GetJournalEditServletFaultInjector is an inner class, let's simply call it FaultInjector.
          • GetJournalEditServletFaultInjector.getInstance() is not used (and it should be static if you want to use it.)
          • change
            new String(path1.toString() + "/current")
            

            to

            "path1 + "/current"
            
          Show
          Tsz Wo Nicholas Sze added a comment - Since GetJournalEditServletFaultInjector is an inner class, let's simply call it FaultInjector. GetJournalEditServletFaultInjector.getInstance() is not used (and it should be static if you want to use it.) change new String (path1.toString() + "/current" ) to "path1 + " /current"
          Hide
          Brandon Li added a comment -

          Todd, it seems to be doable if we make the fault injector class as nested static class.
          In term of the manageability of fault injector classes, people could argue that it might be better to keep all the fault injector classes in a different package. Let me upload the new path and see what other folks think.

          Show
          Brandon Li added a comment - Todd, it seems to be doable if we make the fault injector class as nested static class. In term of the manageability of fault injector classes, people could argue that it might be better to keep all the fault injector classes in a different package. Let me upload the new path and see what other folks think.
          Hide
          Todd Lipcon added a comment -

          Hey Brandon. I personally prefer to keep the fault injector classes local to each individual component being fault-tested. Otherwise it might grow unmanageable over time. They could even become package-private inner classes of each individual class that uses this fault injection technique. I think that's cleanest if doable?

          Show
          Todd Lipcon added a comment - Hey Brandon. I personally prefer to keep the fault injector classes local to each individual component being fault-tested. Otherwise it might grow unmanageable over time. They could even become package-private inner classes of each individual class that uses this fault injection technique. I think that's cleanest if doable?
          Hide
          Brandon Li added a comment -

          So the root cause of the false-OK-response is due to the output stream close operation (response.getOutputStream().close()), which sends false-OK-response to client before HTTP server processes the uncaught exception.

          I did a test with the above mentioned change and also make GetJournalEditServlet.doGet throw an Error.
          Even though this error can't be caught by GetJournalEditServlet.doGet, it is handled by HTTP server and the client can get the error message.

          Thanks, Nicholas.

          Show
          Brandon Li added a comment - So the root cause of the false-OK-response is due to the output stream close operation (response.getOutputStream().close()), which sends false-OK-response to client before HTTP server processes the uncaught exception. I did a test with the above mentioned change and also make GetJournalEditServlet.doGet throw an Error. Even though this error can't be caught by GetJournalEditServlet.doGet, it is handled by HTTP server and the client can get the error message. Thanks, Nicholas.
          Hide
          Tsz Wo Nicholas Sze added a comment -

          The correct way to fix the bug is not to close output stream when an error is sent. Below is a suggestion.

          @@ -140,8 +141,12 @@
                           DataTransferThrottler throttler = GetImageServlet.getThrottler(conf);
           
                           // send edits
          -                TransferFsImage.getFileServer(response.getOutputStream(),
          -                    editFile, throttler);
          +                OutputStream out = response.getOutputStream();
          +                try {
          +                  TransferFsImage.getFileServer(out, editFile, throttler);
          +                } finally {
          +                  out.close();
          +                }
                         } else {
                           response
                               .sendError(HttpServletResponse.SC_METHOD_NOT_ALLOWED,
          @@ -155,8 +160,6 @@
                 String errMsg = "getedit failed. " + StringUtils.stringifyException(ie);
                 response.sendError(HttpServletResponse.SC_GONE, errMsg);
                 throw new IOException(errMsg);
          -    } finally {
          -      response.getOutputStream().close();
               }
             }
          
          Show
          Tsz Wo Nicholas Sze added a comment - The correct way to fix the bug is not to close output stream when an error is sent. Below is a suggestion. @@ -140,8 +141,12 @@ DataTransferThrottler throttler = GetImageServlet.getThrottler(conf); // send edits - TransferFsImage.getFileServer(response.getOutputStream(), - editFile, throttler); + OutputStream out = response.getOutputStream(); + try { + TransferFsImage.getFileServer(out, editFile, throttler); + } finally { + out.close(); + } } else { response .sendError(HttpServletResponse.SC_METHOD_NOT_ALLOWED, @@ -155,8 +160,6 @@ String errMsg = "getedit failed. " + StringUtils.stringifyException(ie); response.sendError(HttpServletResponse.SC_GONE, errMsg); throw new IOException(errMsg); - } finally { - response.getOutputStream().close(); } }
          Hide
          Brandon Li added a comment -

          Renaming it sounds better to me. How about calling it FaultInjector so it can also be used for error simulation in other ears inside HDFS?

          Show
          Brandon Li added a comment - Renaming it sounds better to me. How about calling it FaultInjector so it can also be used for error simulation in other ears inside HDFS?
          Hide
          Todd Lipcon added a comment -

          Good idea. Maybe we should also rename CheckpointFaultInjector to FileTransferFaultInjector or something? Or split it in two, since if I recall correctly some of the fault points are checkpoint-specific whereas others are just for the GetImageServlet.

          Show
          Todd Lipcon added a comment - Good idea. Maybe we should also rename CheckpointFaultInjector to FileTransferFaultInjector or something? Or split it in two, since if I recall correctly some of the fault points are checkpoint-specific whereas others are just for the GetImageServlet.

            People

            • Assignee:
              Brandon Li
              Reporter:
              Brandon Li
            • Votes:
              0 Vote for this issue
              Watchers:
              2 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development