Hadoop Common
  1. Hadoop Common
  2. HADOOP-6686

Remove redundant exception class name in unwrapped exceptions thrown at the RPC client

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 0.21.0
    • Component/s: None
    • Labels:
      None
    • Environment:

      At RPC client, when the exception thrown by the server is unwrapped, the exception message includes redundant exception class name. This redundant information should be removed.

    • Hadoop Flags:
      Incompatible change, Reviewed
    • Release Note:
      The exceptions thrown by the RPC client no longer carries a redundant exception class name in exception message.
    1. HADOOP-6686.1.patch
      2 kB
      Suresh Srinivas
    2. HADOOP-6686.patch
      0.8 kB
      Suresh Srinivas

      Issue Links

        Activity

        Suresh Srinivas created issue -
        Hide
        Suresh Srinivas added a comment -

        RPC server sends exception class name and printed stack trace in response. Proposed change:

        1. if the exception is unwrapped, with the message set to the first line of printed stack trace from response. The message is "<Server exception class name>: <server exception message>"
          • While unwrapping, I propose removing the redundant "<exception class name>: " from the message. Other than HDFSCli test I cannot think of a reason why an application depends on the error string to include exception name, especially given that the exception type that is thrown has that information already.
            #. if the exception is not unwrapped, RemoteException is thrown as it is, with printed stack trace as the message.
          • This behavior will be retained. The exception name here along with the stack trace (albeit the server side) is useful for debugging.
        Show
        Suresh Srinivas added a comment - RPC server sends exception class name and printed stack trace in response. Proposed change: if the exception is unwrapped, with the message set to the first line of printed stack trace from response. The message is "<Server exception class name>: <server exception message>" While unwrapping, I propose removing the redundant "<exception class name>: " from the message. Other than HDFSCli test I cannot think of a reason why an application depends on the error string to include exception name, especially given that the exception type that is thrown has that information already. #. if the exception is not unwrapped, RemoteException is thrown as it is, with printed stack trace as the message. This behavior will be retained. The exception name here along with the stack trace (albeit the server side) is useful for debugging.
        Suresh Srinivas made changes -
        Field Original Value New Value
        Link This issue relates to HDFS-1076 [ HDFS-1076 ]
        Suresh Srinivas made changes -
        Attachment HADOOP-6686.patch [ 12440956 ]
        Suresh Srinivas made changes -
        Link This issue is blocked by HDFS-1083 [ HDFS-1083 ]
        Hide
        Doug Cutting added a comment -

        This is definitely an improvement, but I wonder if it might be better yet if RemoteException didn't include the exception name in its message. It might include it in toString(), as is standard, but not in getMessage(). RemoteException has a field named "className" that already stores the remote exception name. It also redundantly includes the exception name in the exceptions message, since that's created from the value of Throwable#printStackTrace(). Rather RemoteException might parseout just the message (the first line, after the class name) and use that as its message and separately store the remote stack trace in a field, like the class name. Then, even if the exception is not unwrapped, its message and toString() would still match that of the original exception.

        Show
        Doug Cutting added a comment - This is definitely an improvement, but I wonder if it might be better yet if RemoteException didn't include the exception name in its message. It might include it in toString(), as is standard, but not in getMessage(). RemoteException has a field named "className" that already stores the remote exception name. It also redundantly includes the exception name in the exceptions message, since that's created from the value of Throwable#printStackTrace(). Rather RemoteException might parseout just the message (the first line, after the class name) and use that as its message and separately store the remote stack trace in a field, like the class name. Then, even if the exception is not unwrapped, its message and toString() would still match that of the original exception.
        Hide
        Suresh Srinivas added a comment -

        The problem with that approach is, if stack trace is printed for RemoteException (not sure if any does toString() for the exception), then the exception name printed will be RemoteException. The wrapped exception name is lost!

        Show
        Suresh Srinivas added a comment - The problem with that approach is, if stack trace is printed for RemoteException (not sure if any does toString() for the exception), then the exception name printed will be RemoteException. The wrapped exception name is lost!
        Hide
        Suresh Srinivas added a comment -

        After running a quick test, looks like in Throwable#printStackTrace(), the first line printed is Throwable#toString(). This means the change you suggested can be made without losing the wrapped exception info, if we add toString() to RemoteException to print wrapped exception name.

        Show
        Suresh Srinivas added a comment - After running a quick test, looks like in Throwable#printStackTrace(), the first line printed is Throwable#toString(). This means the change you suggested can be made without losing the wrapped exception info, if we add toString() to RemoteException to print wrapped exception name.
        Hide
        Doug Cutting added a comment -

        > add toString() to RemoteException to print wrapped exception name

        +1 I think that's a good approach.

        Show
        Doug Cutting added a comment - > add toString() to RemoteException to print wrapped exception name +1 I think that's a good approach.
        Hide
        Suresh Srinivas added a comment -

        With the change RemoteException has stack trace as exception message without the exception name. However unwrapped exceptions have only the first line from the stack trace without the exception name. Should we change the unwrapped exception to also include the stack trace? I think it will will be good for debugging.

        Show
        Suresh Srinivas added a comment - With the change RemoteException has stack trace as exception message without the exception name. However unwrapped exceptions have only the first line from the stack trace without the exception name. Should we change the unwrapped exception to also include the stack trace? I think it will will be good for debugging.
        Hide
        Suresh Srinivas added a comment -

        Updated patch:

        1. RemoteException message is server side stack trace minus redundant exception name prefix.
        2. Unwrapped exceptions from remote exception has same message as RemoteException, instead of picking only the first line from the server stack trace. The unwrapped message is same as that of RemoteException message.
        Show
        Suresh Srinivas added a comment - Updated patch: RemoteException message is server side stack trace minus redundant exception name prefix. Unwrapped exceptions from remote exception has same message as RemoteException, instead of picking only the first line from the server stack trace. The unwrapped message is same as that of RemoteException message.
        Suresh Srinivas made changes -
        Attachment HADOOP-6686.1.patch [ 12441321 ]
        Hide
        Doug Cutting added a comment -

        +1 This looks good to me.

        Show
        Doug Cutting added a comment - +1 This looks good to me.
        Hide
        Suresh Srinivas added a comment -

        Doug, could you please review and +1 HDFS-1083 as well. Committing both these changes together will keep the window for which HDFS test fails small.

        Show
        Suresh Srinivas added a comment - Doug, could you please review and +1 HDFS-1083 as well. Committing both these changes together will keep the window for which HDFS test fails small.
        Suresh Srinivas made changes -
        Status Open [ 1 ] Patch Available [ 10002 ]
        Suresh Srinivas made changes -
        Status Patch Available [ 10002 ] Open [ 1 ]
        Suresh Srinivas made changes -
        Status Open [ 1 ] Patch Available [ 10002 ]
        Hide
        Hadoop QA added a comment -

        -1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12441321/HADOOP-6686.1.patch
        against trunk revision 937093.

        +1 @author. The patch does not contain any @author tags.

        -1 tests included. The patch doesn't appear to include any new or modified tests.
        Please justify why no new tests are needed for this patch.
        Also please list what manual steps were performed to verify this patch.

        +1 javadoc. The javadoc tool did not generate any warning messages.

        +1 javac. The applied patch does not increase the total number of javac compiler warnings.

        +1 findbugs. The patch does not introduce any new Findbugs warnings.

        +1 release audit. The applied patch does not increase the total number of release audit warnings.

        +1 core tests. The patch passed core unit tests.

        +1 contrib tests. The patch passed contrib unit tests.

        Test results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/474/testReport/
        Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/474/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
        Checkstyle results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/474/artifact/trunk/build/test/checkstyle-errors.html
        Console output: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/474/console

        This message is automatically generated.

        Show
        Hadoop QA added a comment - -1 overall. Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12441321/HADOOP-6686.1.patch against trunk revision 937093. +1 @author. The patch does not contain any @author tags. -1 tests included. The patch doesn't appear to include any new or modified tests. Please justify why no new tests are needed for this patch. Also please list what manual steps were performed to verify this patch. +1 javadoc. The javadoc tool did not generate any warning messages. +1 javac. The applied patch does not increase the total number of javac compiler warnings. +1 findbugs. The patch does not introduce any new Findbugs warnings. +1 release audit. The applied patch does not increase the total number of release audit warnings. +1 core tests. The patch passed core unit tests. +1 contrib tests. The patch passed contrib unit tests. Test results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/474/testReport/ Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/474/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Checkstyle results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/474/artifact/trunk/build/test/checkstyle-errors.html Console output: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/474/console This message is automatically generated.
        Hide
        Suresh Srinivas added a comment -

        Tests are not included in this patch, since it is covered in HDFS-1083. I committed this patch.

        Show
        Suresh Srinivas added a comment - Tests are not included in this patch, since it is covered in HDFS-1083 . I committed this patch.
        Suresh Srinivas made changes -
        Status Patch Available [ 10002 ] Resolved [ 5 ]
        Hadoop Flags [Incompatible change, Reviewed]
        Release Note The exceptions thrown by the RPC client no longer carries a redundant exception class name in exception message.
        Resolution Fixed [ 1 ]
        Vinod Kumar Vavilapalli made changes -
        Link This issue relates to MAPREDUCE-1727 [ MAPREDUCE-1727 ]
        Tom White made changes -
        Fix Version/s 0.21.0 [ 12313563 ]
        Fix Version/s 0.22.0 [ 12314296 ]
        Tom White made changes -
        Status Resolved [ 5 ] Closed [ 6 ]
        Suresh Srinivas made changes -
        Link This issue breaks HADOOP-8661 [ HADOOP-8661 ]

          People

          • Assignee:
            Suresh Srinivas
            Reporter:
            Suresh Srinivas
          • Votes:
            0 Vote for this issue
            Watchers:
            1 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development